WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
78239
Partially loaded JPEGs should have alpha channel
https://bugs.webkit.org/show_bug.cgi?id=78239
Summary
Partially loaded JPEGs should have alpha channel
Sami Kyostila
Reported
2012-02-09 07:34:32 PST
Partially loaded JPEGs should have alpha channel
Attachments
Patch
(3.05 KB, patch)
2012-02-09 07:40 PST
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(8.31 KB, patch)
2012-02-16 22:13 PST
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(8.38 KB, patch)
2012-02-17 10:47 PST
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(8.32 KB, patch)
2012-02-20 12:02 PST
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(8.28 KB, patch)
2012-03-05 06:52 PST
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Sami Kyostila
Comment 1
2012-02-09 07:40:05 PST
Created
attachment 126300
[details]
Patch
Sami Kyostila
Comment 2
2012-02-09 07:42:54 PST
I've tested that this patch fixes the corruption seen in
http://code.google.com/p/chromium/issues/detail?id=113171
.
Kenneth Russell
Comment 3
2012-02-09 10:55:56 PST
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
Comment 4
2012-02-09 11:01:36 PST
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 5
2012-02-09 11:06:56 PST
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
Comment 6
2012-02-09 11:07:46 PST
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
Comment 7
2012-02-09 11:10:40 PST
(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
Comment 8
2012-02-16 22:13:24 PST
Created
attachment 127523
[details]
Patch
Sami Kyostila
Comment 9
2012-02-16 22:22:17 PST
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 10
2012-02-17 01:35:37 PST
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
Comment 11
2012-02-17 10:47:42 PST
Created
attachment 127612
[details]
Patch Move resources out of the way to fix test failure.
Kenneth Russell
Comment 12
2012-02-19 11:20:56 PST
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
Comment 13
2012-02-20 12:02:40 PST
Created
attachment 127840
[details]
Patch
Sami Kyostila
Comment 14
2012-02-20 12:08:50 PST
(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 15
2012-02-21 09:32:47 PST
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
Comment 16
2012-03-05 06:52:14 PST
Created
attachment 130119
[details]
Patch Regenerated changelogs.
Kenneth Russell
Comment 17
2012-03-05 10:08:51 PST
Comment on
attachment 130119
[details]
Patch Looks good.
WebKit Review Bot
Comment 18
2012-03-05 11:55:13 PST
Comment on
attachment 130119
[details]
Patch Clearing flags on attachment: 130119 Committed
r109779
: <
http://trac.webkit.org/changeset/109779
>
WebKit Review Bot
Comment 19
2012-03-05 11:55:20 PST
All reviewed patches have been landed. Closing bug.
noel gordon
Comment 20
2012-03-28 01:12:43 PDT
(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
Comment 21
2012-03-28 01:16:52 PDT
This change regressed WebGL color correction in some cases. Filed
bug 82428
about that: Kenneth could you please review?
Sami Kyostila
Comment 22
2012-03-28 10:32:57 PDT
(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
Comment 23
2012-03-28 12:04:11 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug