Bug 82360 - pass alpha directly to player, rather than creating a layer (for performance)
Summary: pass alpha directly to player, rather than creating a layer (for performance)
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: Mike Reed
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 11:35 PDT by Mike Reed
Modified: 2012-03-30 10:09 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2012-03-27 11:38 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (2.35 KB, patch)
2012-03-27 13:01 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (2.37 KB, patch)
2012-03-28 07:20 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (2.66 KB, patch)
2012-03-29 06:36 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (2.82 KB, patch)
2012-03-30 05:10 PDT, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2012-03-27 11:35:46 PDT
pass alpha directly to player, rather than creating a layer (for performance)
Comment 1 Mike Reed 2012-03-27 11:38:38 PDT
Created attachment 134105 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-27 11:40:23 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 WebKit Review Bot 2012-03-27 11:53:17 PDT
Comment on attachment 134105 [details]
Patch

Attachment 134105 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12143684
Comment 4 Mike Reed 2012-03-27 13:01:37 PDT
Created attachment 134119 [details]
Patch
Comment 5 Mike Reed 2012-03-27 13:06:55 PDT
need to wait for webkit's DEPS on chrome to update to get the api change already landed there...
Comment 6 WebKit Review Bot 2012-03-27 13:24:59 PDT
Comment on attachment 134119 [details]
Patch

Attachment 134119 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12148612
Comment 7 Mike Reed 2012-03-28 07:20:57 PDT
Created attachment 134290 [details]
Patch
Comment 8 Mike Reed 2012-03-28 09:08:15 PDT
Comment on attachment 134290 [details]
Patch

mis-fired on the +. Just meant to say ?
Comment 9 Mike Reed 2012-03-28 11:19:08 PDT
PTAL
Comment 10 Darin Fisher (:fishd, Google) 2012-03-28 15:24:28 PDT
Comment on attachment 134290 [details]
Patch

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

> Source/WebKit/chromium/public/WebMediaPlayer.h:119
> +    virtual void paint(WebCanvas* canvas, const WebRect& rect)

nit: Can you delete this old method and just fix-up the WEBKIT_USING_CG path to pass 0xFF?
Comment 11 James Robinson 2012-03-28 16:19:01 PDT
Can we just delete the WEBKIT_USING_CG code from **/chromium/** completely? We don't compile in this mode and are unlikely to start doing so spontaneously.
Comment 12 Adam Barth 2012-03-28 17:12:10 PDT
> Can we just delete the WEBKIT_USING_CG code from **/chromium/** completely? We don't compile in this mode and are unlikely to start doing so spontaneously.

My understanding is that we now delete WEBKIT_USING_CG.  It's untested, therefore it's broken.
Comment 13 Mike Reed 2012-03-29 05:42:13 PDT
I would prefer to delete dead code unrelated to this change, in a separate CL, so I don't conflate the two ideas (and slightly complicate testing and revert possibilities). However, if deleting that code in just these files will get me approved, then I'll do it :)
Comment 14 Mike Reed 2012-03-29 06:36:23 PDT
Created attachment 134565 [details]
Patch
Comment 15 Mike Reed 2012-03-29 06:37:37 PDT
this patch also deletes (dead) code behind WEBKIT_USING_CG, and assumes WEBKIT_USING_SKIA
Comment 16 Mike Reed 2012-03-29 07:10:59 PDT
PTAL
Comment 17 James Robinson 2012-03-29 14:37:18 PDT
Comment on attachment 134565 [details]
Patch

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

I don't know enough about what this does to review it. You will need to find someone who is familiar with this code.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:45
>  // WebCommon.h defines WEBKIT_USING_SKIA so this has to be included last.
> -#if WEBKIT_USING_SKIA
>  #include "PlatformContextSkia.h"

now that you've removed the #ifdef you should delete the comment and put this in the normal include location (sorted with other includes)
Comment 18 Andrew Scherkus 2012-03-29 15:05:03 PDT
jamesr: I've been working with reed on the chromium-side changes

patch is r+ to me
Comment 19 James Robinson 2012-03-29 16:49:32 PDT
Comment on attachment 134565 [details]
Patch

OK, that works for me (and would have been useful information to have on this bug earlier)
Comment 20 Mike Reed 2012-03-30 05:10:27 PDT
Created attachment 134785 [details]
Patch
Comment 21 Mike Reed 2012-03-30 05:17:06 PDT
#include moved into upper section with comment removed.

PTAL
Comment 22 Stephen White 2012-03-30 08:12:51 PDT
Comment on attachment 134785 [details]
Patch

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

Looks good.  r=me

> Source/WebKit/chromium/ChangeLog:3
> +        pass alpha directly to player, rather than creating a layer (for performance)

Super-nit:  This should probably be a real sentence.
Comment 23 WebKit Review Bot 2012-03-30 10:09:10 PDT
Comment on attachment 134785 [details]
Patch

Clearing flags on attachment: 134785

Committed r112682: <http://trac.webkit.org/changeset/112682>
Comment 24 WebKit Review Bot 2012-03-30 10:09:16 PDT
All reviewed patches have been landed.  Closing bug.