Bug 88218 - [chromium] Use WEBPImportPictureRGBX|BGRX to import picture data
Summary: [chromium] Use WEBPImportPictureRGBX|BGRX to import picture data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-04 01:51 PDT by noel gordon
Modified: 2012-06-08 22:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (23.22 KB, patch)
2012-06-04 02:24 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch (22.83 KB, patch)
2012-06-05 19:52 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch (21.05 KB, patch)
2012-06-07 08:23 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch for landing (21.07 KB, patch)
2012-06-07 17:53 PDT, noel gordon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2012-06-04 01:51:34 PDT
Part of a two-sided patch: https://chromiumcodereview.appspot.com/10496016 defines WEBPImportPictureRGBX|BGRX for importing picture data ignoring the alpha channel.  WebKit should use these methods to import data for encoding when creating a image/webp dataURL.  The methods WebKit currently uses, namely WEBPImportPictureRGBA|BGRA, will include the alpha channel during import in a future release of libwebp so we should stop using these methods to future-proof the code.
Comment 1 noel gordon 2012-06-04 02:24:52 PDT
Created attachment 145546 [details]
Patch
Comment 2 noel gordon 2012-06-04 02:30:33 PDT
Waiting on DEPS http://src.chromium.org/viewvc/chrome?view=rev&revision=140264 to roll into webkit.
Comment 3 Pascal Massimino 2012-06-04 05:50:55 PDT
lgtm, but one need to wait for the new functions to be in the official libwebp tree (release 0.1.4?) before using them...

btw: there's a missing checksum reported for the second PNG
Comment 4 noel gordon 2012-06-05 01:18:00 PDT
(In reply to comment #3)
> lgtm, but one need to wait for the new functions to be in the official libwebp tree (release 0.1.4?) before using them...

Actually, I just need chromium DEPS to rev 140264 to use the functions since they are in the chromium repository.
 
> btw: there's a missing checksum reported for the second PNG

I generated the png result with run-webkit-tests, that should create a checksum, and the error report says "create the png with run-webkit-tests" :)

Dunno what's going on there.
Comment 5 noel gordon 2012-06-05 01:29:26 PDT
http://trac.webkit.org/changeset/119468 chromium DEPS rolled.
Comment 6 noel gordon 2012-06-05 19:52:24 PDT
Created attachment 145918 [details]
Patch
Comment 7 noel gordon 2012-06-05 20:38:42 PDT
Since the linux EWS are backed up, built chromium-linux and ran the new test.  Works fines.
Comment 8 Kent Tamura 2012-06-05 23:35:45 PDT
Comment on attachment 145918 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * fast/canvas/canvas-toDataURL-webp-no-alpha-expected.txt: Added.
> +        * fast/canvas/canvas-toDataURL-webp-no-alpha.html: Added.

Can you make this a reftest?
Comment 9 noel gordon 2012-06-05 23:56:58 PDT
Maybe, prefer to see the composite onto black pixels here.
Comment 10 noel gordon 2012-06-07 08:20:04 PDT
Discussed the existing test fast/canvas/canvas-toDataURL-webp.html with tkent, and we agree to replace it with the new test.
Comment 11 noel gordon 2012-06-07 08:23:59 PDT
Created attachment 146291 [details]
Patch
Comment 12 Kent Tamura 2012-06-07 16:47:39 PDT
Comment on attachment 146291 [details]
Patch

ok
Comment 13 WebKit Review Bot 2012-06-07 17:38:35 PDT
Comment on attachment 146291 [details]
Patch

Rejecting attachment 146291 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12908507
Comment 14 noel gordon 2012-06-07 17:53:55 PDT
Created attachment 146437 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-06-07 19:57:46 PDT
Comment on attachment 146437 [details]
Patch for landing

Clearing flags on attachment: 146437

Committed r119787: <http://trac.webkit.org/changeset/119787>
Comment 16 WebKit Review Bot 2012-06-07 19:58:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 noel gordon 2012-06-07 21:14:50 PDT
(In reply to comment #4)
>
> > btw: there's a missing checksum reported for the second PNG
> 
> I generated the png result with run-webkit-tests, that should create a checksum, and the error report says "create the png with run-webkit-tests" :)
> 
> Dunno what's going on there.

Bug 88368.
Comment 18 noel gordon 2012-06-07 23:24:32 PDT
Added rebaselines on http://trac.webkit.org/changeset/119801
Comment 19 Ojan Vafai 2012-06-08 00:00:41 PDT
FYI, as per garden-o-matic, the MISSING line in TestExpectations needs to include Lion and you need a line for the equivalent virtual gpu test platform/chromium/virtual/gpu/fast/canvas/canvas-toDataURL-webp.html.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2Fcanvas-toDataURL-webp.html
Comment 20 noel gordon 2012-06-08 00:24:01 PDT
http://trac.webkit.org/changeset/119804 for the virtual GPU.
Comment 21 noel gordon 2012-06-08 01:36:37 PDT
(In reply to comment #19)
> FYI, as per garden-o-matic, the MISSING line in TestExpectations needs to include Lion

Want lion: so optimize http://trac.webkit.org/changeset/119812