[webkit-reviews] review denied: [Bug 73043] Teach VideoLayerChromium how to render native texture (to support HW video decode). : [Attachment 119876] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Ami Fischman
<fischman at chromium.org>'s request for review:
Bug 73043: Teach VideoLayerChromium how to render native texture (to support HW
video decode).
https://bugs.webkit.org/show_bug.cgi?id=73043

Attachment 119876: Patch
https://bugs.webkit.org/attachment.cgi?id=119876&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119876&action=review


This looks good except for the case where the CCVideoLayerImpl dies before its
provider. It looks like you have all the API surface necessary to handle this,
but it just isn't quite wired up. R- for that and the "No new tests (OOPS!)."
in the ChangeLog but other than that I think this is ready to land.

> Source/WebCore/ChangeLog:3
> +	   Fix the life-cycle of video frames handled by
VideoLayerChromium/CCVideoLayerImpl.

the preferred structure for ChangeLog entries is:

dateline

	one-line description of the patch (typically the bug title)
	https://bugs.webkit.org/show_bug.cgi?id=12345

	Reviewed by NOBODY (OOPS!).

	long form description of the patch, typically broken over
	multiple lines

	description of test coverage

	* list of files

> Source/WebCore/ChangeLog:14
> +	   No new tests. (OOPS!)

this will fail an svn presubmit hook, you have to remove it from the ChangeLog
entry yourself and replace it with a description of the test coverage for this
patch (like the exiting compositing/video/ tests, for instance)

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.h:57
> +    virtual void stopUsingProvider();

nit: could you document that this is an implementation of the
VideoFrameProvider::Client interface?

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:74
> +    ASSERT(!m_provider); // Cleared during
VideoLayerChromium::releaseProvider().

I believe this comment is out of date, since
VideoLayerChromium::releaseProvider() isn't relevant to this codepath any more.


What is supposed to happen if the CCVideoLayerChromium is destroyed before the
VideoFrameProvider? I would expect that cause the client to call
setVideoFrameProviderClient(0) on the provider, but it doesn't seem that's what
is happening here.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:87
> +static GC3Denum convertVFCFormatToGC3DFormat(VideoFrameChromium::Format
vfcFormat)

we typically don't use abbreviations like 'vfc' in variable/parameter names in
WebKit. Here I think that 'format' would be a fine variable name

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.h:57
> +    virtual void stopUsingProvider();

could you document that this is an implementation of
VideoFrameProvider::Client, and that it can be called from any thread?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:624
> +    client->m_videoFrameProviderClient = 0;

is there a strong reason to set this here instead of inside the
WebMediaPlayerClientImpl c'tor?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:164
> +    Mutex m_compositingMutex; // Guards interaction between the main thread
and compisiting thread.

typo 'compisiting'->'compositing'

ideally mutexes should guard data. could you document what data this mutex
guards (it looks like it's guarding the m_videoFrameProviderClient pointer and
potentially m_currentideoFrame, if I'm reading this correctly)