Bug 172670 – REGRESSION(r216882): No image decoding is needed if the BitmapImage is created with a NativeImage

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

If the BitmapImage is created with a NativeImage that means there is no ImageDecoder will be created. The reason we were going through the async image decoding path is the size of the image frame is smaller than the destination rectangle. In this case, the NativeImage has to be drawn as is and the drawing will scale it to the destination rectangle.

[Said Abou-Hallawa]

Created attachment 311395 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/32384008>

[Said Abou-Hallawa]

Created attachment 311397 [details]
Patch

[Said Abou-Hallawa]

Created attachment 311399 [details]
test case (will crash)

[Tim Horton]

Comment on attachment 311397 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        REGRESSION(r216882): Disable async image for the BitmapImage which is created with a NativeImage

Async image what?

> Source/WebCore/ChangeLog:11
> +        We need to check whether the ImageDecoder has been created for the BitmapImage
> +        before going through the async image decoding code path.

s/the ImageDecoder/an ImageDecoder/, maybe?

Also I don't really understand why it's OK to do anything called "decoding" if no decoder is available (why is non-async-decoding OK?).

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:317
>      ASSERT(isDecoderAvailable());

Were we not hitting this assert?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:319
> +    if (!isDecoderAvailable())
> +        return;

Why do you need this? Shouldn't the early return in shouldUseAsyncDecoding() be enough? And the assert here?

[Said Abou-Hallawa]

Comment on attachment 311397 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        REGRESSION(r216882): Disable async image for the BitmapImage which is created with a NativeImage
> 
> Async image what?

Title was changed to: No image decoding is needed if the BitmapImage is created with a NativeImage.

>> Source/WebCore/ChangeLog:11
>> +        before going through the async image decoding code path.
> 
> s/the ImageDecoder/an ImageDecoder/, maybe?
> 
> Also I don't really understand why it's OK to do anything called "decoding" if no decoder is available (why is non-async-decoding OK?).

I mentioned the async image decoding because it is initiated by WebKit while the sync image decode is initiated only while committing the page drawing.

The comment has changed to say: Check whether the BitmapImage has created an ImageDecoder before trying to decode its image frame.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:317
>>      ASSERT(isDecoderAvailable());
> 
> Were we not hitting this assert?

The assertion fires in debug builds. In release builds, we are just crashing.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:319
>> +        return;
> 
> Why do you need this? Shouldn't the early return in shouldUseAsyncDecoding() be enough? And the assert here?

You are right. But I thought adding this extra early return here makes the code more robust in case the caller side itself gets changed. But I will remove this one.

[Said Abou-Hallawa]

Created attachment 311524 [details]
Patch

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #6)
> Comment on attachment 311397 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311397&action=review
> >> Source/WebCore/platform/graphics/ImageFrameCache.cpp:319
> >> +        return;
> > 
> > Why do you need this? Shouldn't the early return in shouldUseAsyncDecoding() be enough? And the assert here?
> 
> You are right. But I thought adding this extra early return here makes the
> code more robust in case the caller side itself gets changed. But I will
> remove this one.

Generally if you see "ASSERT(something); if (!something) return;", you're doing something silly (either it's expected that it can be null and you shouldn't have the assert, or it's a programming error if it is null and the assert is sufficient) :) Thank you for removing.

[WebKit Commit Bot]

Comment on attachment 311524 [details]
Patch

Clearing flags on attachment: 311524

Committed r217567: <http://trac.webkit.org/changeset/217567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.