Bug 73858 - [chromium] Set opaque() for VideoLayerChromium
Summary: [chromium] Set opaque() for VideoLayerChromium
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: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-05 13:04 PST by Dana Jansens
Modified: 2011-12-08 15:01 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.68 KB, patch)
2011-12-05 13:32 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (2.57 KB, patch)
2011-12-05 15:36 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2011-12-06 11:49 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (2.50 KB, patch)
2011-12-07 08:13 PST, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2011-12-05 13:04:13 PST
Sets opaque in WebMediaPlayerClientImpl.
Comment 1 Dana Jansens 2011-12-05 13:32:23 PST
Created attachment 117933 [details]
Patch

Two questions here.

1. Should WebVideoFrame::isFormatOpaque() be virtual?

2. Should the "if (layer->opaque() != opaque)" test be done in LC::setOpaque() or in the caller of the function?
Comment 2 WebKit Review Bot 2011-12-05 13:36:17 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-12-05 14:11:55 PST
Comment on attachment 117933 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:3
> +        [chromium] Set opaque() for VideoLayerChromium

This ChangeLog summary seems inconsistent with the contents of the patch.

> Source/WebKit/chromium/public/WebVideoFrame.h:65
> +    virtual bool isFormatOpaque()

do you mean for this function to be overridden by the implementation class?
if not, then perhaps this should not be virtual.  you may also want to define
this non-inline since it generates a non-trivial amount of code.  or, perhaps
this should just be a static helper function defined in WebMediaPlayerClientImpl.cpp?
Comment 4 Dana Jansens 2011-12-05 14:47:35 PST
Comment on attachment 117933 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:3
>> +        [chromium] Set opaque() for VideoLayerChromium
> 
> This ChangeLog summary seems inconsistent with the contents of the patch.

I'm not sure what you mean here. See third comment.

>> Source/WebKit/chromium/public/WebVideoFrame.h:65
>> +    virtual bool isFormatOpaque()
> 
> do you mean for this function to be overridden by the implementation class?
> if not, then perhaps this should not be virtual.  you may also want to define
> this non-inline since it generates a non-trivial amount of code.  or, perhaps
> this should just be a static helper function defined in WebMediaPlayerClientImpl.cpp?

I was not sure if it should be over-ridden or not. I suppose a subclass can not extend the Format enumeration, so it would make sense to not be virtual. This code migrated from https://bugs.webkit.org/show_bug.cgi?id=72964 where it was suggested to stick it in the analogous VideoFrameChromium, to keep it close to the list of formats. I think that makes sense here too. Would you accept a static helper function here instead?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:584
> +                m_videoLayer->setOpaque(opaque);

Here we are setting the opaque flag on the VideoLayerChromium.
Comment 5 Darin Fisher (:fishd, Google) 2011-12-05 15:19:31 PST
We should only add methods to the WebKit API that are either required by Chromium to call into WebKit, or required by WebKit to call back up to Chromium.  In this case, it looks like the WebKit implementation doesn't need Chromium to provide an answer to a question, so the question can be answered directly by WebKit.
Comment 6 Dana Jansens 2011-12-05 15:36:59 PST
Created attachment 117949 [details]
Patch

Thanks for feedback darin. New patch without change to WebVideoFrame.h.

Would still appreciate feedback on Question 2 above. Thanks.
Comment 7 James Robinson 2011-12-05 17:04:50 PST
Comment on attachment 117949 [details]
Patch

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

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:578
> +    default:
> +        ASSERT_NOT_REACHED();

if you want to ensure that this switch has a case for every possible value of the WebVideoFrame::Format enum, a better way to do it is to leave out the 'default' case completely - then the compiler will throw a warning (which is an error with our compiler settings) if there's an enum value without a possible case:

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:595
> +}

i think you will need to add a return here outside the switch(), preferably with an ASSERT_NOT_REACHED() as well, or some versions of gcc we use will complain about a missing return statement in a non-void function

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:607
> +            if (opaque != m_videoLayer->opaque())
> +                m_videoLayer->setOpaque(opaque);

it doesn't seem very useful to do the state checking here since the implementation of setOpaque() will also compare the new state against the old and trigger the appropriate changes if necesssary I'd cut this down to just one line that calls setOpaque() on m_videoLayer with the appropriate value (something like "m_videoLayer->setOpaque(webkitVideoFrame && frameFormatOpaque(...)").
Comment 8 Dana Jansens 2011-12-06 07:19:13 PST
Comment on attachment 117949 [details]
Patch

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

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:578
>> +        ASSERT_NOT_REACHED();
> 
> if you want to ensure that this switch has a case for every possible value of the WebVideoFrame::Format enum, a better way to do it is to leave out the 'default' case completely - then the compiler will throw a warning (which is an error with our compiler settings) if there's an enum value without a possible case:

ok cool. saw this used elsewhere, but would prefer no default as you say.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:595
>> +}
> 
> i think you will need to add a return here outside the switch(), preferably with an ASSERT_NOT_REACHED() as well, or some versions of gcc we use will complain about a missing return statement in a non-void function

ok.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:607
>> +                m_videoLayer->setOpaque(opaque);
> 
> it doesn't seem very useful to do the state checking here since the implementation of setOpaque() will also compare the new state against the old and trigger the appropriate changes if necesssary I'd cut this down to just one line that calls setOpaque() on m_videoLayer with the appropriate value (something like "m_videoLayer->setOpaque(webkitVideoFrame && frameFormatOpaque(...)").

I don't believe setOpaque() checks the new value. it just sets it and calls setNeedsCommit() regardless. I was trying to avoid doing that every frame, but can put the check in setOpaque() instead if you'd like?
Comment 9 David Reveman 2011-12-06 08:42:37 PST
(In reply to comment #8)
> (From update of attachment 117949 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117949&action=review
> 
> >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:578
> >> +        ASSERT_NOT_REACHED();
> > 
> > if you want to ensure that this switch has a case for every possible value of the WebVideoFrame::Format enum, a better way to do it is to leave out the 'default' case completely - then the compiler will throw a warning (which is an error with our compiler settings) if there's an enum value without a possible case:
> 
> ok cool. saw this used elsewhere, but would prefer no default as you say.
> 
> >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:595
> >> +}
> > 
> > i think you will need to add a return here outside the switch(), preferably with an ASSERT_NOT_REACHED() as well, or some versions of gcc we use will complain about a missing return statement in a non-void function
> 
> ok.
> 
> >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:607
> >> +                m_videoLayer->setOpaque(opaque);
> > 
> > it doesn't seem very useful to do the state checking here since the implementation of setOpaque() will also compare the new state against the old and trigger the appropriate changes if necesssary I'd cut this down to just one line that calls setOpaque() on m_videoLayer with the appropriate value (something like "m_videoLayer->setOpaque(webkitVideoFrame && frameFormatOpaque(...)").
> 
> I don't believe setOpaque() checks the new value. it just sets it and calls setNeedsCommit() regardless. I was trying to avoid doing that every frame, but can put the check in setOpaque() instead if you'd like?

Hm, should setOpaque really call setNeedsCommit()? Isn't opaque-ness just additional information about the layer that can optionally be used for rendering? It doesn't affect output, just affects the process we use to produce the output. Shouldn't setNeedsCommit() just be called from functions such as setOpacity, which actually affect output?
Comment 10 James Robinson 2011-12-06 11:24:43 PST
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerChromium.cpp#L288

LC::setOpaque() and LC::setOpacity() both call setNeedsCommit(), which I believe is correct in both cases - the opaque bit may be used when rendering to control blending and draw-time culling.
Comment 11 Dana Jansens 2011-12-06 11:34:26 PST
> I don't believe setOpaque() checks the new value. it just sets it and calls setNeedsCommit() regardless. I was trying to avoid doing that every frame, but can put the check in setOpaque() instead if you'd like?

Oh, thanks for the trac. I see this changed recently, yay. Will fix.
Comment 12 Dana Jansens 2011-12-06 11:49:41 PST
Created attachment 118082 [details]
Patch

Updated with requested fixes.
Comment 13 Dana Jansens 2011-12-06 14:44:24 PST
I see this failed on win bot, but seems unrelated to the patch. Not sure how to resubmit to the bot without re-uploading.
Comment 14 Darin Fisher (:fishd, Google) 2011-12-06 15:23:31 PST
Comment on attachment 118082 [details]
Patch

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

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:573
> +static bool frameFormatOpaque(WebVideoFrame::Format format)

nit: please do not put global functions in the middle of a string
of class member function definitions.  please move global functions
to the top of the source file.

nit: the name "frameFormatOpaque" could be improved.  it seems like
this function is trying to ask a question, so it should probably be
phrased in the form of a question.  how about "isFormatOpaque" or
"isVideoFrameFormatOpaque"?
Comment 15 Dana Jansens 2011-12-07 08:13:06 PST
Created attachment 118213 [details]
Patch

Nits addressed, thanks.
Comment 16 James Robinson 2011-12-07 17:33:08 PST
Comment on attachment 118213 [details]
Patch

Looks good to me.
Comment 17 WebKit Review Bot 2011-12-08 12:34:12 PST
Comment on attachment 118213 [details]
Patch

Rejecting attachment 118213 [details] from commit-queue.

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

Last 500 characters of output:
ebKit/chromium/third_party/yasm/source/patched-yasm --revision 73761 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
48>At revision 73761.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/10781409
Comment 18 WebKit Review Bot 2011-12-08 15:01:11 PST
Comment on attachment 118213 [details]
Patch

Clearing flags on attachment: 118213

Committed r102387: <http://trac.webkit.org/changeset/102387>
Comment 19 WebKit Review Bot 2011-12-08 15:01:17 PST
All reviewed patches have been landed.  Closing bug.