Bug 73858 – [chromium] Set opaque() for VideoLayerChromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

Sets opaque in WebMediaPlayerClientImpl.

[Dana Jansens]

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?

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

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?

[Dana Jansens]

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.

[Darin Fisher (:fishd, Google)]

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.

[Dana Jansens]

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.

[James Robinson]

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(...)").

[Dana Jansens]

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?

[David Reveman]

(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?

[James Robinson]

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.

[Dana Jansens]

> 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.

[Dana Jansens]

Created attachment 118082 [details]
Patch

Updated with requested fixes.

[Dana Jansens]

I see this failed on win bot, but seems unrelated to the patch. Not sure how to resubmit to the bot without re-uploading.

[Darin Fisher (:fishd, Google)]

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"?

[Dana Jansens]

Created attachment 118213 [details]
Patch

Nits addressed, thanks.

[James Robinson]

Comment on attachment 118213 [details]
Patch

Looks good to me.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 118213 [details]
Patch

Clearing flags on attachment: 118213

Committed r102387: <http://trac.webkit.org/changeset/102387>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.