Bug 44539 – API changes for Video Frame sharing between WebKit and Chromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

API changes for Video Frame sharing between WebKit and Chromium

[Victoria Kirst]

Created attachment 65297 [details]
Patch

[Victoria Kirst]

Hi Darin! Could you take a look at this patch when you have time? I tried to address the concerns you and others mentioned in the previous review.

Some key points:

* This patch only contains the API changes and doesn't actually use the classes/methods yet. This is both so that it is hopefully easier to review, and also because I need to land a patch on the Chromium side before I can use these classes without breaking the build.

* WebVideoFrame interface does not contain enums and instead returns "unsigned" for the format/surface type. WebVideoFrame is intended to be a proxy between VideoFrameChromium and media::VideoFrame anyway, so it seemed like a reasonable trade-off to eliminate the enums from WebVideoFrame. 

* I didn't add anything to AssertMatchingEnums.cpp because (in light of the change mentioned above) I need to match enums between WebCore and Chromium, not WebCore and WebKit. Do you know of a way to do this?

[Victoria Kirst]

Created attachment 65306 [details]
Patch

[Victoria Kirst]

Comment on attachment 65306 [details]
Patch

Changed a few straggler size_t -> unsigned

[Victoria Kirst]

Created attachment 65337 [details]
Patch

[Victoria Kirst]

Comment on attachment 65337 [details]
Patch

Fixed some style issues brought up from levin. Thanks again! :)

[Victoria Kirst]

Created attachment 65439 [details]
Patch

[Victoria Kirst]

Comment on attachment 65439 [details]
Patch

Added enums back to WebVideoFrame, and added compiler asserts to match WebVideoFrame and VideoFrameChromium enums.

[Darin Fisher (:fishd, Google)]

Comment on attachment 65439 [details]
Patch

WebCore/platform/graphics/chromium/VideoFrameChromium.h:38
 +  
nit: extra new line here

WebCore/platform/graphics/chromium/VideoFrameChromium.h:71
 +  public:
nit: no need for the redundant 'public:' section

WebCore/platform/graphics/chromium/VideoFrameProvider.h:39
 +  
nit: extra new line here

WebCore/platform/graphics/chromium/VideoFrameProvider.h:41
 +      virtual VideoFrameChromium* getCurrentFrame() = 0;
please document the ownership model.  who owns the VideoFrameChromium?
how do I free it?  it looks like i have to call putCurrentFrame to
free it, but is that the only way?  it might help if these methods
were more fully documented.

WebKit/chromium/public/WebMediaPlayer.h:133
 +      // FIXME: Will be made pure virtual when API changes are fully landed.
WebKit API interfaces implemented by the embedder are intended to have
non-pure virtual methods.  so this FIXME is backwards.  the other methods
should have empty implementations :)

WebKit/chromium/public/WebVideoFrame.h:40
 +  
nit: kill the extra new line here

WebKit/chromium/public/WebVideoFrame.h:43
 +      enum Format {
nit: our convention for enums at the WebKit API level is to use the following
format:

  enum Foo {
      FooBar,
      FooBaz,
      ...
  };

So, I'd expect to see:

  enum Format {
      FormatInvalid,
      FormatRGB555,
      ...
  };

  enum SurfaceType {
      SurfaceTypeSystemMemory,
      ...
  };

WebKit/chromium/public/WebVideoFrame.h:66
 +      virtual SurfaceType type() const = 0;
nit: type should be surfaceType to match the type name SurfaceType

WebKit/chromium/public/WebVideoFrame.h:71
 +      virtual int stride(unsigned plane) const = 0;
why does stride return signed instead of unsigned?

WebKit/chromium/public/WebVideoFrame.h:72
 +      virtual void* data(unsigned plane) const = 0;
can this pointer be a |const void*| instead?  it should be const
if you only expect the caller to get read access to the buffer.
otherwise, non-const implies mutability.  i think you only want
to provide read access, right?

WebKit/chromium/src/VideoFrameChromiumImpl.cpp:55
 +  VideoFrameChromiumImpl::VideoFrameChromiumImpl(WebVideoFrame* webVideoFrame) :
nit: the colon should be on the next line

WebKit/chromium/src/VideoFrameChromiumImpl.h:42
 +  
nit: no new line here

WebKit/chromium/src/VideoFrameChromiumImpl.cpp:47
 +  WebVideoFrame* VideoFrameChromiumImpl::toWebVideoFrame(WebCore::VideoFrameChromium* videoFrame)
nit: i recommend adding a 'using namespace WebCore' at the top of this file,
and then remove the WebCore:: prefixes in the body of this file.

WebKit/chromium/src/VideoFrameChromiumImpl.cpp:56
 +      m_webVideoFrame(webVideoFrame)
what is the ownership model for m_webVideoFrame?  it looks like VideoFrameChromiumImpl
is not responsible for deleting m_webVideoFrame.  should it?

WebKit/chromium/src/WebMediaPlayerClientImpl.h:119
 +      // This function returns a pointer to a VideoFrameChromium, which is
this looks like documentation that should be in VideoFrameProvider.h instead of here.

[Victoria Kirst]

Created attachment 65477 [details]
Patch

[Victoria Kirst]

Hi Darin! I fixed my code to match your suggestions. Can you take another look?

> WebCore/platform/graphics/chromium/VideoFrameProvider.h:41
>  +      virtual VideoFrameChromium* getCurrentFrame() = 0;
> please document the ownership model.  who owns the VideoFrameChromium?
> how do I free it?  it looks like i have to call putCurrentFrame to
> free it, but is that the only way?  it might help if these methods
> were more fully documented.
I moved the comment from WebMediaPlayerClientImpl.h to this file. Let me know if you think I should add further clarification.

> WebKit/chromium/public/WebMediaPlayer.h:133
>  +      // FIXME: Will be made pure virtual when API changes are fully landed.
> WebKit API interfaces implemented by the embedder are intended to have
> non-pure virtual methods.  so this FIXME is backwards.  the other methods
> should have empty implementations :)
For now I just deleted the FIXME.

> WebKit/chromium/public/WebVideoFrame.h:71
>  +      virtual int stride(unsigned plane) const = 0;
> why does stride return signed instead of unsigned?
Strides can be negative if a frame is stored bottom-up in memory, and I believe this is true of RGB frames. (Not positive, though.)

> WebKit/chromium/src/VideoFrameChromiumImpl.cpp:56
>  +      m_webVideoFrame(webVideoFrame)
> what is the ownership model for m_webVideoFrame?  it looks like VideoFrameChromiumImpl
> is not responsible for deleting m_webVideoFrame.  should it?
You're correct, VideoFrameChromiumImpl is not responsible for deleting m_webVideoFrame. I added a comment saying such.

[Darin Fisher (:fishd, Google)]

Comment on attachment 65477 [details]
Patch

WebKit/chromium/public/WebMediaPlayer.h:132
 +      // For getting frames to and from media player.
nit: Something like the comments on VideoFrameProvider should be here as well.
Ideally, the user of the WebKit API should not need to read WebCore files to
understand how to use the API.

WebKit/chromium/public/WebVideoFrame.h:31
 +  
nit: extra new line here that should be removed

WebKit/chromium/public/WebVideoFrame.h:41
 +      // These enums must be kept in sync with media::VideoFormat.
nit: I'd prefer that we avoid references to code in the Chromium repository.
These things can easily get out of sync as folks changing code in Chromium
may not realize they need to make a patch to WebKit to fix up the name of
something.  I'd just drop this comment.

WebKit/chromium/src/VideoFrameChromiumImpl.h:46
 +      static WebVideoFrame* toWebVideoFrame(VideoFrameChromium*);
nit: i'd recommend a new line after here for readability

R=me w/ these nits fixed.

[Victoria Kirst]

Created attachment 65512 [details]
Patch

[Victoria Kirst]

Thanks for the review, Darin! I will have Alpha commit it for me.