Summary: | HTMLImageElement.decode() should optimize the decoding for the image explicit size | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||||||||||||||
Component: | Images | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||||||||||||||
Status: | NEW --- | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, dbates, dino, esprehn+autocc, gyuyoung.kim, japhet, mjs, rniwa, simon.fraser, thorton | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2017-10-06 18:05:01 PDT
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.
|