Bug 174479 – Make the decision for asynchronously decoding an image be in one place

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

Currently RenderBoxModelObject::decodingModeForImageDraw() chooses the DecodingMode based on the view type and whether the FrameView is printing or not. BitmapImage::draw() consults both the DecodingMode and the settings to make the final decision. What looks weird also is BitmapImage::draw() does not use the DecodingMode, it receives, for animated images.

We need to have the decision of whether an image should be asynchronously decoded or not be in one place. The DecodingMode received by BitmapImage::draw() should be used by both the animated and the large image cases. This will make it easy for future changes.

[Said Abou-Hallawa]

Created attachment 315385 [details]
Patch

[Build Bot]

Comment on attachment 315385 [details]
Patch

Attachment 315385 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4115904

New failing tests:
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 315386 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315385 [details]
Patch

Attachment 315385 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4115969

New failing tests:
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 315389 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315385 [details]
Patch

Attachment 315385 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4115923

New failing tests:
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 315391 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 315392 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #0)
> Currently RenderBoxModelObject::decodingModeForImageDraw() chooses the
> DecodingMode based on the view type and whether the FrameView is printing or
> not. BitmapImage::draw() consults both the DecodingMode and the settings to
> make the final decision. What looks weird also is BitmapImage::draw() does
> not use the DecodingMode, it receives, for animated images.
> 
I was wrong in the comment. The DecodingMode argument to BitmapImage::draw() is used for the drawing the current frame which has to be synchronous for the animated image. The DecodingMode for the next frame has to be calculated by BitmapImage::internalStartAnimation().

[Tim Horton]

Comment on attachment 315392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315392&action=review

> Source/WebCore/loader/cache/CachedImage.cpp:150
> -        // In this case, the client we are asked to add is the root box renderer. Since we can't add
> +        // In this case, the client, we are asked to add, is the root box renderer. Since we can't add

This is a grammar regression.

[Said Abou-Hallawa]

Created attachment 315442 [details]
Patch

[Said Abou-Hallawa]

Created attachment 315470 [details]
Patch

[Tim Horton]

Comment on attachment 315470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315470&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:324
> +    if (IOSApplication::isIBooks())

Given recent discussions, this should be more specific (definitely about just iBooks Storytime, but also possibly other details -- I'll show you the email thread).

> Source/WebCore/rendering/RenderImage.cpp:588
> +    auto drawResult = paintInfo.context().drawImage(*img, rect, ImagePaintingOptions(compositeOperator, BlendModeNormal, decodingModeForImageDraw(*image, paintInfo), orientationDescription, interpolation));

what is going on with "image" and "img" in this function? I hate it!

[Said Abou-Hallawa]

Created attachment 315526 [details]
Patch

[Build Bot]

Comment on attachment 315526 [details]
Patch

Attachment 315526 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4124130

New failing tests:
http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html

[Build Bot]

Created attachment 315534 [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 315615 [details]
Patch

[Build Bot]

Comment on attachment 315615 [details]
Patch

Attachment 315615 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4131960

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315624 [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.5

[Said Abou-Hallawa]

Something is wrong with iOS simulator bots. It has been failing because of different reasons which seems unrelated to this patch. 

The first time, http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html failed because of the following crash:

0   libsystem_kernel.dylib        	0x000000010f3d0d42 __pthread_kill + 10
1   libsystem_pthread.dylib       	0x000000010f408457 pthread_kill + 90
2   libsystem_c.dylib             	0x000000010f16288f abort + 127
3   libc++abi.dylib               	0x000000010d61cce5 abort_message + 245
4   libc++abi.dylib               	0x000000010d637cbd default_terminate_handler() + 265
5   libobjc.A.dylib               	0x000000010cbdd3c4 _objc_terminate() + 103
6   libc++abi.dylib               	0x000000010d634f29 std::__terminate(void (*)()) + 8
7   libc++abi.dylib               	0x000000010d634bbe __cxa_rethrow + 99
8   libobjc.A.dylib               	0x000000010cbdd2dc objc_exception_rethrow + 40
9   com.apple.CoreFoundation      	0x000000010d1ab096 CFRunLoopRunSpecific + 534
10  com.apple.GraphicsServices    	0x000000010eb0ea24 GSEventRunModal + 62
11  com.apple.UIKit               	0x000000010a2e40d4 UIApplicationMain + 159
12  org.webkit.WebKitTestRunnerApp	0x00000001082f752d main + 97 (mainIOS.mm:73)

The second time, imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html failed because of this crash:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000109b4ff16 WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTaskReply() + 70 (memory:2726)
1   com.apple.WebCore             	0x0000000108fb1bb9 WebCore::IDBServer::IDBServer::handleTaskRepliesOnMainThread() + 89 (memory:2537)
2   JavaScriptCore                	0x000000010823acf5 WTF::dispatchFunctionsFromMainThread() + 181 (memory:2537)
3   com.apple.Foundation          	0x0000000101202e51 __NSThreadPerformPerform + 334
4   com.apple.CoreFoundation      	0x0000000102819c01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
5   com.apple.CoreFoundation      	0x00000001027ff0cf __CFRunLoopDoSources0 + 527
6   com.apple.CoreFoundation      	0x00000001027fe5ff __CFRunLoopRun + 911
7   com.apple.CoreFoundation      	0x00000001027fe016 CFRunLoopRunSpecific + 406
8   com.apple.Foundation          	0x00000001011b6480 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 274
9   com.apple.Foundation          	0x00000001011b635b -[NSRunLoop(NSRunLoop) run] + 76

[WebKit Commit Bot]

Comment on attachment 315615 [details]
Patch

Clearing flags on attachment: 315615

Committed r219548: <http://trac.webkit.org/changeset/219548>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Comment on attachment 315470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315470&action=review

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:324
>> +    if (IOSApplication::isIBooks())
> 
> Given recent discussions, this should be more specific (definitely about just iBooks Storytime, but also possibly other details -- I'll show you the email thread).

I changed this to be IOSApplication::isIBooksStorytime(). The proposal about narrowing down scope of the application specific enabling/disabling code has not been implemented yet. My understanding about this proposal is we need to check the { application, version } to make the right decision. So it should be outside the scope of this patch.

>> Source/WebCore/rendering/RenderImage.cpp:588
>> +    auto drawResult = paintInfo.context().drawImage(*img, rect, ImagePaintingOptions(compositeOperator, BlendModeNormal, decodingModeForImageDraw(*image, paintInfo), orientationDescription, interpolation));
> 
> what is going on with "image" and "img" in this function? I hate it!

Fot BitmapImage case, image and img should be the same object. They may differ for SVG images. SVGImageCache has a map { renderer -> Image }. And RenderImageResource::image() returns the Image object for the RenderImage. For the decoding case we can use either of image or img; there is no difference.

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #23)
> Comment on attachment 315470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315470&action=review
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:324
> >> +    if (IOSApplication::isIBooks())
> > 
> > Given recent discussions, this should be more specific (definitely about just iBooks Storytime, but also possibly other details -- I'll show you the email thread).
> 
> I changed this to be IOSApplication::isIBooksStorytime(). The proposal about
> narrowing down scope of the application specific enabling/disabling code has
> not been implemented yet. My understanding about this proposal is we need to
> check the { application, version } to make the right decision. So it should
> be outside the scope of this patch.

Sure.

> >> Source/WebCore/rendering/RenderImage.cpp:588
> >> +    auto drawResult = paintInfo.context().drawImage(*img, rect, ImagePaintingOptions(compositeOperator, BlendModeNormal, decodingModeForImageDraw(*image, paintInfo), orientationDescription, interpolation));
> > 
> > what is going on with "image" and "img" in this function? I hate it!
> 
> Fot BitmapImage case, image and img should be the same object. They may
> differ for SVG images. SVGImageCache has a map { renderer -> Image }. And
> RenderImageResource::image() returns the Image object for the RenderImage.
> For the decoding case we can use either of image or img; there is no
> difference.

Can we make their names less insanely confusing? :)