Consider the following scenarios: 1) Changing the css background-image of an element from a low resolution image to a higher resolution image when it finishes loading. 2) Slide show where the src of the <img> element is changed when a user click on the next and the previous buttons. 3) A webcam image where the cached image is recreated every time a new frame is pushed from the server to WebKit. In these cases we should not use async image decoding because it is two steps drawing: 1) the first time, the image frame is requested for decoding. 2) the second time, the image is drawn after its frame is decoded. Because during the first step no contents will be displayed, a flash will happen. If no content is displayed in the middle of displaying the same cached image twice or displaying two different cached images, this will be considered a flash. So we have to ensure nothing was drawn in the renderer rectangle before being in situation we are going to display no contents where there was an image drawn in the same rectangle. In other words we should detect the case of flashing before it happens and don't use the asynchronous image decoding in this case. A very reliable way to detect this case is to consult the tile repaintCount. If it is zero, then it is safe to use asynchronous image decoded. If the tile repaintCount is greater than zero, we are not sure if the renderer rectangle has an image drawn in it already or not. In this case we have to use the synchronous image decoding to avoid causing a flash.
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
<rdar://problem/31246421>
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>
*** Bug 174337 has been marked as a duplicate of this bug. ***
*** Bug 175138 has been marked as a duplicate of this bug. ***