Bug 78239 – Partially loaded JPEGs should have alpha channel

• Component: Images
• Status: RESOLVED FIXED

[Sami Kyostila]

Partially loaded JPEGs should have alpha channel

[Sami Kyostila]

Created attachment 126300 [details]
Patch

[Sami Kyostila]

I've tested that this patch fixes the corruption seen in http://code.google.com/p/chromium/issues/detail?id=113171.

[Kenneth Russell]

Comment on attachment 126300 [details]
Patch

Will all clients of this code properly pay attention to the transition from translucent to opaque? What happens if a JPEG image ends up in its own composited layer, for example?

[James Robinson]

We don't create composited layers for images until the image is fully loaded (specifically until isLoaded() returns true, triggered by the notifyFinished() callback).  So I think we're fine on that front.

Any way to get test coverage for this, perhaps by using an http test configured to serve only part of the image?

[Simon Fraser (smfr)]

Comment on attachment 126300 [details]
Patch

I'm not sure I agree with this. Isn't it up to image clients to only draw the loaded part of the image?

[Sami Kyostila]

If some clients miss the transition to opaque, then they're not worse off than they are now since they have been treating the image transparent anyway -- otherwise they would have seen the corruption during the load.

Testing should be possible if we can control the http response, or if there is some other way to load an image partially, say, from a data URL.

[Sami Kyostila]

(In reply to comment #5)
> I'm not sure I agree with this. Isn't it up to image clients to only draw the loaded part of the image?

I agree that it would be more efficient to only draw the valid part of the image, but this seems like a fair bit of complexity for a relatively small gain. The optimization would only kick in while the image is being loaded.

[Sami Kyostila]

Created attachment 127523 [details]
Patch

[Sami Kyostila]

This new version adds a test for rendering a partially loaded JPEG. I fear it might not be entirely deterministic due to the lack of synchronization between the image loading and rendering; a new callback or additional polling could fix that.

I still plan to investigate propagating the loaded region up to the client, but please feel free to review the test in the meanwhile.

[WebKit Review Bot]

Comment on attachment 127523 [details]
Patch

Attachment 127523 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11540403

New failing tests:
http/tests/loading/partial-jpeg.php

[Sami Kyostila]

Created attachment 127612 [details]
Patch

Move resources out of the way to fix test failure.

[Kenneth Russell]

Comment on attachment 127612 [details]
Patch

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

The code and test look good overall, but the test needs a little more work in my opinion.

> LayoutTests/http/tests/loading/partial-jpeg-expected.txt:4
> +main frame - didFailLoadWithError

Are these diagnostic outputs really expected to be in the result?

> LayoutTests/http/tests/loading/partial-jpeg.html:16
> +    setTimeout(forceDisplay, 0);

Based on the presence of the "didFailLoadWithError" diagnostic output above, can you use an onError callback on the image tag in order to reliably detect when it's loaded as far as it's going to, and launch the rest of the test then?

Also, I think you need to use layoutTestController.waitUntilDone() / layoutTestController.notifyDone() to further avoid race conditions in this test case.

[Sami Kyostila]

Created attachment 127840 [details]
Patch

[Sami Kyostila]

(In reply to comment #12)
> > LayoutTests/http/tests/loading/partial-jpeg-expected.txt:4
> > +main frame - didFailLoadWithError
> 
> Are these diagnostic outputs really expected to be in the result?

Nope, looks like they got enabled automatically since the test was under loading/. I've now moved the test under incremental/.

> > LayoutTests/http/tests/loading/partial-jpeg.html:16
> > +    setTimeout(forceDisplay, 0);
> 
> Based on the presence of the "didFailLoadWithError" diagnostic output above, can you use an onError callback on the image tag in order to reliably detect when it's loaded as far as it's going to, and launch the rest of the test then?

That's a good suggestion, but turns out the load failure is triggered by window.stop() and not by the image tag. onError never fires for the image, since the load just stalls and doesn't actually fail.

> Also, I think you need to use layoutTestController.waitUntilDone() / layoutTestController.notifyDone() to further avoid race conditions in this test case.

Good idea, done.

[Kenneth Russell]

Comment on attachment 127840 [details]
Patch

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

Thanks, looks good overall. Please regenerate the ChangeLogs and I'll r+ it. BTW, please mark it cq? if you want it submitted to the commit queue.

> Source/WebCore/ChangeLog:25
> +               http/tests/loading/partial-jpeg.php

Please regenerate the ChangeLogs with the new test names.

> LayoutTests/ChangeLog:23
> +        * http/tests/loading/resources/checkerboard.jpg: Added.

These are out of date now.

[Sami Kyostila]

Created attachment 130119 [details]
Patch

Regenerated changelogs.

[Kenneth Russell]

Comment on attachment 130119 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 130119 [details]
Patch

Clearing flags on attachment: 130119

Committed r109779: <http://trac.webkit.org/changeset/109779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #6)
> If some clients miss the transition to opaque, then they're not worse off than they are now since they have been treating the image transparent anyway -- otherwise they would have seen the corruption during the load.

You won't see the corruption unless an image is large and is being decoded during load.  And then only with skia change r3036 so far.

One has to wonder how/why skia is going around ImageSource::frameHasAlphaAtIndex()
  http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ImageSource.cpp?rev=109779#L173

or wonder if the bug would reproduce for a large, opaque PNG
   http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp?rev=109779#L369

No test for PNG here, and the JPEG test needs fixing according the dashboard.

[noel gordon]

This change regressed WebGL color correction in some cases.  Filed bug 82428 about that: Kenneth could you please review?

[Sami Kyostila]

(In reply to comment #20)
> One has to wonder how/why skia is going around ImageSource::frameHasAlphaAtIndex()
>   http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ImageSource.cpp?rev=109779#L173

That is indeed a good question. I'll look into it.
 
> or wonder if the bug would reproduce for a large, opaque PNG
>    http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp?rev=109779#L369

Right, looks like the bug should happen for PNGs too.
 
> No test for PNG here, and the JPEG test needs fixing according the dashboard.

Opened bug 82480 for this.

[Sami Kyostila]

(In reply to comment #22)
> (In reply to comment #20)
> > One has to wonder how/why skia is going around ImageSource::frameHasAlphaAtIndex()
> >   http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ImageSource.cpp?rev=109779#L173
> 
> That is indeed a good question. I'll look into it.

Looks like ImageSkia.cpp doesn't consult currentFrameHasAlpha() and instead draws the underlying SkBitmap blindly. Seems like an optimization opportunity to me. I'll put together a patch.