Sets opaque in WebMediaPlayerClientImpl.
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?
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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 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.
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.
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 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 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?
(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?
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.
> 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.
Created attachment 118082 [details] Patch Updated with requested fixes.
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 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"?
Created attachment 118213 [details] Patch Nits addressed, thanks.
Comment on attachment 118213 [details] Patch Looks good to me.
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 on attachment 118213 [details] Patch Clearing flags on attachment: 118213 Committed r102387: <http://trac.webkit.org/changeset/102387>
All reviewed patches have been landed. Closing bug.