If the HTMLImageElement has an intrinsic size, the decode() method should use it instead of decoding the image for the native size. The intrinsic size of an HTMLImageElement can be set to the width and the height attributes of the <img> element. Example is: <img width="200" height="100> Or it can be set from the javascript. Example is: var image = new Image(200, 100); If the intrinsic size is used, decoding the image and drawing it will be much faster than using the native size. This is because a large image will not be scaled down when it is drawn into a smaller rectangle.
Created attachment 323074 [details] Patch
Comment on attachment 323074 [details] Patch Attachment 323074 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4787123 New failing tests: fast/images/decode-animated-image.html fast/images/decode-render-animated-image.html
Created attachment 323078 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 323074 [details] Patch Attachment 323074 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4787039 New failing tests: fast/images/decode-animated-image.html fast/images/decode-render-animated-image.html
Created attachment 323079 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 323074 [details] Patch Attachment 323074 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4787204 New failing tests: fast/images/decode-animated-image.html fast/images/decode-render-animated-image.html
Created attachment 323081 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 323074 [details] Patch Attachment 323074 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4788850 New failing tests: fast/images/decode-animated-image.html fast/images/decode-render-animated-image.html
Created attachment 323096 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 323158 [details] Patch
Created attachment 323192 [details] Patch
Created attachment 324757 [details] Patch
Comment on attachment 324757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324757&action=review > Source/WebCore/html/HTMLImageElement.cpp:440 > + return std::nullopt; I think this can be 'return { }' > Source/WebCore/html/HTMLImageElement.cpp:447 > + return std::nullopt; Ditto. > Source/WebCore/html/HTMLImageElement.h:53 > + std::optional<unsigned> intrinsicWidth() const; > + std::optional<unsigned> intrinsicHeight() const; I'm not super keen on the names of these functions because "intrinsic size" has a specific meaning in CSS, and this isn't the same; all it does is read the attribute if present. Maybe this is more like "explicitWidth" or "widthFromAttribute"? > Source/WebCore/platform/graphics/BitmapImage.cpp:516 > + // Animated images should be decoded with the native size always. Comment should say why. > Source/WebCore/platform/graphics/BitmapImage.cpp:543 > + Vector<size_t> itemsToBeDeleted; This is a vector of indexes, not of items (or requests). > Source/WebCore/platform/graphics/BitmapImage.cpp:544 > + for (auto it = m_decodingRequests->begin(); it != m_decodingRequests->end(); ++it) { Use a modern C++ loop: for (auto request : *m_decodingRequests) ? > Source/WebCore/platform/graphics/BitmapImage.cpp:549 > + it->callback(); What if the callback modifies m_decodingRequests? I think you should copy m_decodingRequests before iterating it. > Source/WebCore/platform/graphics/BitmapImage.cpp:562 > + m_decodingRequests->remove(itemsToBeDeleted.takeLast()); Is this removing by index or by item? Confused.
Created attachment 324945 [details] Patch
Isn’t the wording here wrong? You’re now deciding at the target resolution, explicitly NOT the intrinsic size of the image.
Comment on attachment 324757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324757&action=review >> Source/WebCore/html/HTMLImageElement.cpp:440 >> + return std::nullopt; > > I think this can be 'return { }' Done. >> Source/WebCore/html/HTMLImageElement.cpp:447 >> + return std::nullopt; > > Ditto. Done. >> Source/WebCore/html/HTMLImageElement.h:53 >> + std::optional<unsigned> intrinsicHeight() const; > > I'm not super keen on the names of these functions because "intrinsic size" has a specific meaning in CSS, and this isn't the same; all it does is read the attribute if present. Maybe this is more like "explicitWidth" or "widthFromAttribute"? Names were changed to explicitWidth() and explicitHeight(). >> Source/WebCore/platform/graphics/BitmapImage.cpp:516 >> + // Animated images should be decoded with the native size always. > > Comment should say why. The comment was changed. >> Source/WebCore/platform/graphics/BitmapImage.cpp:543 >> + Vector<size_t> itemsToBeDeleted; > > This is a vector of indexes, not of items (or requests). The name of vector was renamed to indicesOfItemsToBeDeleted. >> Source/WebCore/platform/graphics/BitmapImage.cpp:544 >> + for (auto it = m_decodingRequests->begin(); it != m_decodingRequests->end(); ++it) { > > Use a modern C++ loop: for (auto request : *m_decodingRequests) ? Done. I used the classic for-loop because I wanted to calculate the index of the element = it - m_decodingRequests->begin(). I now calculate the index of the element = &request - &m_decodingRequests->first(). >> Source/WebCore/platform/graphics/BitmapImage.cpp:549 >> + it->callback(); > > What if the callback modifies m_decodingRequests? I think you should copy m_decodingRequests before iterating it. There is only caller for the decode() method which is ImageLoader::decode(). The ImageLoader passes a callback which resolves a promise. Resolving the promise can't result in reentering BitmapImage::decode(). Drain asked this question before and I answered it in https://bugs.webkit.org/show_bug.cgi?id=176732#c5. >> Source/WebCore/platform/graphics/BitmapImage.cpp:562 >> + m_decodingRequests->remove(itemsToBeDeleted.takeLast()); > > Is this removing by index or by item? Confused. Name of the vector was changed to indicesOfItemsToBeDeleted.
(In reply to Tim Horton from comment #15) > Isn’t the wording here wrong? You’re now deciding at the target resolution, > explicitly NOT the intrinsic size of the image. Deciding=decoding, and I see smfr raised the same complaint. Bug title is still wrong, though.
Created attachment 324946 [details] Patch
Created attachment 325028 [details] Patch
Comment on attachment 324946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324946&action=review Can we test this change? > Source/WebCore/platform/graphics/BitmapImage.cpp:554 > + for (const auto& request : *m_decodingRequests) { > + if (!frameHasDecodedNativeImageCompatibleWithOptionsAtIndex(m_currentFrame, m_currentSubsamplingLevel, request.sizeForDrawing)) > + continue; > + > + // Call the request callback and mark this request for deletion. > + request.callback(); > + indicesOfItemsToBeDeleted.append(&request - &m_decodingRequests->first()); Since you're getting the index here, the loop would be much cleaner as just for (size_t i = 0; i < m_decodingRequests.size(); ++i) You say that calling the callback while traversing m_decodingRequests is safe because promise resolution isn't synchronous. However, this doesn't protect us against our own coding mistakes in C++ where we later change our code to modify m_decodingRequests inside the callback. I think you should copy the vector. > Source/WebCore/platform/graphics/BitmapImage.cpp:566 > + while (!indicesOfItemsToBeDeleted.isEmpty()) > + m_decodingRequests->remove(indicesOfItemsToBeDeleted.takeLast()); Why bother to mutate (indicesOfItemsToBeDeleted? Just run through the list. > Source/WebCore/platform/graphics/BitmapImage.h:175 > + bool hasStartedAnimating() const { return bool(m_desiredFrameStartTime); } I would find m_desiredFrameStartTime != 0 more readable.
Comment on attachment 325028 [details] Patch Attachment 325028 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5001405 New failing tests: fast/xsl/xslt-enc.xml
Created attachment 325043 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 325065 [details] Patch
Comment on attachment 324946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324946&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:554 >> + indicesOfItemsToBeDeleted.append(&request - &m_decodingRequests->first()); > > Since you're getting the index here, the loop would be much cleaner as just for (size_t i = 0; i < m_decodingRequests.size(); ++i) > > You say that calling the callback while traversing m_decodingRequests is safe because promise resolution isn't synchronous. However, this doesn't protect us against our own coding mistakes in C++ where we later change our code to modify m_decodingRequests inside the callback. I think you should copy the vector. I changed the for loop as suggested. But I could not copy the Vector m_decodingRequests because the WTF::Function is not copyable. I tried to think of all the cases that can wrong with m_decodingRequests and I realized even using a vector of indices is wrong. If callDecodingCallbacks() is reentrant from callDecodingCallbacks() this will mess up all the indices. I tried to come up with a solution which can stand up against all the bugs which may introduce because of mistakes in our C++ code. Then I felt it's too much complicated for bug that we know for sure it does not exist. Instead I can add assertion that callDecodingCallbacks() is not reentrant from callDecodingCallbacks() decode() is not reentrant from either decode() or callDecodingCallbacks() >> Source/WebCore/platform/graphics/BitmapImage.cpp:566 >> + m_decodingRequests->remove(indicesOfItemsToBeDeleted.takeLast()); > > Why bother to mutate (indicesOfItemsToBeDeleted? Just run through the list. Done. >> Source/WebCore/platform/graphics/BitmapImage.h:175 >> + bool hasStartedAnimating() const { return bool(m_desiredFrameStartTime); } > > I would find m_desiredFrameStartTime != 0 more readable. Done.
Created attachment 325071 [details] Patch
Attachment 325071 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/BitmapImage.h:175: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 325094 [details] Patch
Created attachment 325171 [details] Patch
Comment on attachment 324946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324946&action=review >>> Source/WebCore/platform/graphics/BitmapImage.h:175 >>> + bool hasStartedAnimating() const { return bool(m_desiredFrameStartTime); } >> >> I would find m_desiredFrameStartTime != 0 more readable. > > Done. It has to be m_desiredFrameStartTime != MonotonicTime() because the conversion constructor MonotonicTime(double rawValue) is private.
Created attachment 325640 [details] Patch
(In reply to Said Abou-Hallawa from comment #30) > Created attachment 325640 [details] > Patch Tim suggested a solution for protecting m_decodingRequests while calling the decoding callbacks. We can move m_decodingRequests to a local protectedDecodingRequests. See which ones are compatible with the decoded frame. Call their callbacks. Delete these compatible requests from the local protectedDecodingRequests. Move the rest back to m_decodingRequests once we finish. I implemented this solution in this patch.
Comment on attachment 325640 [details] Patch Unfortunately, this old patch no longer applies.