Summary: | Async image decoding for large images should be disabled after the first time a tile is painted | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||||||
Component: | Images | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, frank, jlewis3, jonlee, rniwa, simon.fraser, zalan | ||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=187374 | ||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||
Bug Blocks: | 174653 | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2017-07-12 18:56:46 PDT
Created attachment 315496 [details]
Patch
Created attachment 315504 [details]
Patch
(In reply to Said Abou-Hallawa from comment #2) > Created attachment 315504 [details] > Patch Notice that this patch includes the patch of https://bugs.webkit.org/show_bug.cgi?id=174479 since it has not been reviewed yet. I will update the patch of this bug once the other patch is landed. Comment on attachment 315504 [details] Patch Attachment 315504 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4122596 New failing tests: http/tests/canvas/canvas-tainted-after-draw-image.html Created attachment 315519 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 315639 [details]
Patch
Comment on attachment 315639 [details] Patch Attachment 315639 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4134131 New failing tests: storage/websql/execute-sql-rowsAffected.html http/tests/canvas/canvas-tainted-after-draw-image.html Created attachment 315646 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 315678 [details]
Patch
Comment on attachment 315678 [details] Patch Attachment 315678 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4136691 New failing tests: security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html Created attachment 315687 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315678 [details] Patch Attachment 315678 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4137137 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/historical.html imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html imported/w3c/web-platform-tests/html/the-xhtml-syntax/parsing-xhtml-documents/xhtml-mathml-dtd-entity-7.htm Created attachment 315704 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315678&action=review > Source/WebCore/platform/graphics/ca/TileGrid.cpp:776 > + return m_tileRepaintCounts.contains(platformCALayer) ? m_tileRepaintCounts.get(platformCALayer) : 0; This is two hash lookups. You could do it in one with find(). > Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:1129 > + flags = GraphicsLayerPaintFlags::None; Would be more self-documenting, and correct, as flags &= ~AllowAsyncImageDecoding > Source/WebCore/rendering/RenderBoxModelObject.cpp:330 > if (document().isImageDocument()) > return DecodingMode::Synchronous; > + if (bitmapImage.isLargeImageAsyncDecodingEnabledForTesting()) > + return DecodingMode::Asynchronous; Maybe the testing flag should trump the image document case, so we can test everything? > Source/WebCore/rendering/RenderBoxModelObject.cpp:337 > + if (!isVisibleInViewport()) isVisibleInViewport is not cheap. Maybe add a FIXME noting that. > Source/WebCore/rendering/RenderLayerBacking.cpp:2584 > + flags = GraphicsLayerPaintFlags::None; Would be more self-documenting, and correct, as flags &= ~AllowAsyncImageDecoding Created attachment 315754 [details]
Patch
Created attachment 315760 [details]
Patch
Comment on attachment 315760 [details] Patch Clearing flags on attachment: 315760 Committed r219610: <http://trac.webkit.org/changeset/219610> All reviewed patches have been landed. Closing bug. *** Bug 173298 has been marked as a duplicate of this bug. *** This caused an API failure with test: SnapshotImageLargeAsyncDecoding on all platforms. https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/2088 https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/2088/steps/run-api-tests/logs/stdio Talked to Said, rolling out for now. Reverted r219610 for reason: This caused an api failure on all platforms for the test SnapshotImageLargeAsyncDecoding Committed r219620: <http://trac.webkit.org/changeset/219620> Created attachment 315950 [details]
Patch
Attachment 315950 [details] did not pass style-queue:
Traceback (most recent call last):
File "Tools/Scripts/check-webkit-style", line 48, in <module>
sys.exit(CheckWebKitStyle().main())
File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 155, in main
patch_checker.check(patch)
File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
self._text_file_reader.process_file(file_path=path, line_numbers=None)
File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
self._files[abs_file_path] = self._files[abs_file_path] + kwargs['line_numbers']
TypeError: can only concatenate list (not "NoneType") to list
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Build Bot from comment #23) > Attachment 315950 [details] did not pass style-queue: > > > Traceback (most recent call last): > File "Tools/Scripts/check-webkit-style", line 48, in <module> > sys.exit(CheckWebKitStyle().main()) > File > "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line > 155, in main > patch_checker.check(patch) > File > "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader. > py", line 71, in check > self._text_file_reader.process_file(file_path=path, line_numbers=None) > File > "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", > line 118, in process_file > self._files[abs_file_path] = self._files[abs_file_path] + > kwargs['line_numbers'] > TypeError: can only concatenate list (not "NoneType") to list > > > If any of these errors are false positives, please file a bug against > check-webkit-style. I am not sure what are these errors. I ran Tools/Scripts/check-webkit-style locally and I got "Total errors found: 0 in 67 files". Comment on attachment 315950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315950&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=174451 Please include Radar number here Comment on attachment 315950 [details]
Patch
I need to apply this patch to the trunk and upload a newer patch.
Created attachment 316236 [details]
Patch
Attachment 316236 [details] did not pass style-queue:
Traceback (most recent call last):
File "Tools/Scripts/check-webkit-style", line 48, in <module>
sys.exit(CheckWebKitStyle().main())
File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 155, in main
patch_checker.check(patch)
File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
self._text_file_reader.process_file(file_path=path, line_numbers=None)
File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 119, in process_file
self._files[abs_file_path] = self._files[abs_file_path] + kwargs['line_numbers']
TypeError: can only concatenate list (not "NoneType") to list
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 316236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316236&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:221 > + LOG(Images, "BitmapImage::%s - %p - url: %s [++++++++++++++++++++ an asynchronous decoded frame is available for drawing]", __FUNCTION__, this, sourceURL().string().utf8().data()); I don't think this is a useful change. > Source/WebCore/platform/graphics/BitmapImage.cpp:241 > + LOG(Images, "BitmapImage::%s - %p - url: %s [+-+-+-+-+-+-+-+-+-+- an asynchronous decoded frame will used for synchronous drawing]", __FUNCTION__, this, sourceURL().string().utf8().data()); +-+-+-+-+-+-+-+-+-+- and -------------------- strike me as the result of a logging "arms race". Please don't commit them. > Source/WebCore/platform/graphics/GraphicsLayerClient.h:81 > + GraphicsLayerPaintTileFirstPaint = 1 << 1, I would call this GraphicsLayerPaintFirstTilePaint Comment on attachment 316236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316236&action=review > Source/WebCore/rendering/RenderBoxModelObject.cpp:340 > + // FIXME: isVisibleInViewport() is not cheap. Find a way to make this condition faster. > + if (!isVisibleInViewport()) Since decodingModeForImageDraw() is called during painting and painting already has computed absolute coordinates (paint container abs), it's really wasteful to walk on the ancestor chain again to resolve relative coordinates for each image at every paint. Is this a measurable impact on PLT or MotionMark? Created attachment 316336 [details]
Patch
Comment on attachment 316236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316236&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:221 >> + LOG(Images, "BitmapImage::%s - %p - url: %s [++++++++++++++++++++ an asynchronous decoded frame is available for drawing]", __FUNCTION__, this, sourceURL().string().utf8().data()); > > I don't think this is a useful change. Removed. >> Source/WebCore/platform/graphics/BitmapImage.cpp:241 >> + LOG(Images, "BitmapImage::%s - %p - url: %s [+-+-+-+-+-+-+-+-+-+- an asynchronous decoded frame will used for synchronous drawing]", __FUNCTION__, this, sourceURL().string().utf8().data()); > > +-+-+-+-+-+-+-+-+-+- and -------------------- strike me as the result of a logging "arms race". Please don't commit them. plus and minus signs were removed. >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:81 >> + GraphicsLayerPaintTileFirstPaint = 1 << 1, > > I would call this GraphicsLayerPaintFirstTilePaint Name was changed. >> Source/WebCore/rendering/RenderBoxModelObject.cpp:340 >> + if (!isVisibleInViewport()) > > Since decodingModeForImageDraw() is called during painting and painting already has computed absolute coordinates (paint container abs), it's really wasteful to walk on the ancestor chain again to resolve relative coordinates for each image at every paint. Is this a measurable impact on PLT or MotionMark? You are right. I will address this issue in a separate patch since converting from container absolute coordinates to document absolute coordinates is not super straightforward. I ran PLT and MotionMark and I can confirm that this patch did not cause a regression in any of them. Comment on attachment 316336 [details] Patch Clearing flags on attachment: 316336 Committed r219876: <http://trac.webkit.org/changeset/219876> All reviewed patches have been landed. Closing bug. *** Bug 174337 has been marked as a duplicate of this bug. *** *** Bug 175138 has been marked as a duplicate of this bug. *** |