Bug 174451 – Async image decoding for large images should be disabled after the first time a tile is painted

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 315496 [details]
Patch

[Said Abou-Hallawa]

Created attachment 315504 [details]
Patch

[Said Abou-Hallawa]

(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.

[Build Bot]

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

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 315639 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 315678 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Simon Fraser (smfr)]

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

[Said Abou-Hallawa]

Created attachment 315754 [details]
Patch

[Said Abou-Hallawa]

Created attachment 315760 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315760 [details]
Patch

Clearing flags on attachment: 315760

Committed r219610: <http://trac.webkit.org/changeset/219610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

*** Bug 173298 has been marked as a duplicate of this bug. ***

[Matt Lewis]

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.

[Matt Lewis]

Reverted r219610 for reason:

This caused an api failure on all platforms for the test SnapshotImageLargeAsyncDecoding

Committed r219620: <http://trac.webkit.org/changeset/219620>

[Said Abou-Hallawa]

Created attachment 315950 [details]
Patch

[Build Bot]

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.

[Said Abou-Hallawa]

(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".

[Jon Lee]

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

[Jon Lee]

<rdar://problem/31246421>

[Said Abou-Hallawa]

Comment on attachment 315950 [details]
Patch

I need to apply this patch to the trunk and upload a newer patch.

[Said Abou-Hallawa]

Created attachment 316236 [details]
Patch

[Build Bot]

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.

[Simon Fraser (smfr)]

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

[zalan]

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?

[Said Abou-Hallawa]

Created attachment 316336 [details]
Patch

[Said Abou-Hallawa]

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.

[WebKit Commit Bot]

Comment on attachment 316336 [details]
Patch

Clearing flags on attachment: 316336

Committed r219876: <http://trac.webkit.org/changeset/219876>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

*** Bug 174337 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

*** Bug 175138 has been marked as a duplicate of this bug. ***