Implementing video frame sharing between WebKit and Chromium
Created attachment 63661 [details] Patch
This patch is not intended be committed as is; it is composed of a chromium side (http://codereview.chromium.org/3058055) in addition to the WebKit side so I will need to break it up several smaller pieces in order to keep things compiling on both sides.
Attachment 63661 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3621550
This is a patch that contains the proposed plumbing between WebKit and Chromium to give VideoLayerChromium direct access to the video frames in the Chromium side. As a proof of concept, the Y layer is output to a texture, so running this patch (and the corresponding Chromium patch) with --enable-accelerated-compositing should output a black-and-white video to the screen. Some parts are a bit rough around the edges still, but I wanted to put the patch up for code review/discussion before completely polishing things. The patch requires a bit of explanation. To remind everyone what we're doing: In the VideoLayerChromium, we wanted to have direct access to the uncompressed YUV frame data such that we could copy this data directly to textures and use the GPU to do color conversion. The uncompressed YUV frame data lies in the VideoRendererImpl, which is in chromium. I drew a diagram here to help with the explanation: * https://docs.google.com/a/google.com/drawings/edit?id=15THug8YVpvTgp0KPkW4mIJG0ZTG-qRL862Ovlf-ciW0&hl=en (Let me know if you have trouble accessing it.) VideoLayerChromium needed a way to ask for frames. I wrote an interface called WebCore::VideoFrameProvider that consists of getCurrentFrame and putCurrentFrame methods, analogous to media::VideoFrame's get/put methods. WebMediaPlayerClientImpl implements the WebCore::VideoFrameProvider interface, as it has access to VideoRendererImpl indirectly through m_webMediaPlayer, and it creates and stores a reference to VideoLayerChromium. When a VideoLayerChromium is created, it is given a reference to its owning WebMediaPlayerClientImpl and stores it as a VideoFrameProvider* m_provider. It then uses m_provider to get/put frames when needed. One of the trickiest parts of implementing this plumbing was due to the data type for the VideoFrame and the strict divisions in code between WebKit/WebKit/chromium/public, WebKit/WebCore/, and webkit/glue. VideoLayerChromium needs to be able to access the video frame, so there had to be a WebCore::VideoFrameChromium type. However, WebMediaPlayerClientImpl can only communicate to VideoRendererImpl through m_webMediaPlayer, which implements the WebKit::WebMediaPlayer interface. This WebKit interface cannot use WebCore types, and we also try to avoid WebCore types in chromium. Thus to make the communication possible, I decided to create both a WebCore::VideoFrameChromium interface and a WebKit::WebVideoFrame interface. These interfaces are virtually identical other than their namespaces. Thus to make all the types happy: I wrote a WebKit::WebVideoFrame interface that is implemented by webkit_glue::WebVideoFrameImpl and is simply a wrapper for media::VideoFrame (takes a VideoFrame* in the constructor). I then wrote a WebCore::VideoFrameChromium interface implemented by WebKit::VideoFrameChromiumImpl that is a wrapper for WebKit::WebVideoFrame (takes a WebVideoFrame* in the constructor). What this means is there are 3 parts of the code between WebKit, WebCore, and chromium that are unfortunately duplicated: WebKit::WebVideoFrame, WebCore::VideoFrameChromium, and media::VideoFrame. This actually wouldn't be *that* bad since the method headers copied are not likely going to change, *but* I also have to duplicate the enums and constants. That is really going to suck to try to keep the enums synchronized in the 3 places. Any tips on how to clean this part up would be greatly appreciated! You can refer to the diagram to see how these pieces all work together. The entire patch is available for review here: http://codereview.chromium.org/3058055 Since I'm not going to commit this patch as-is anyway, I am not really looking for line-by-line feedback as I am looking for feedback on the overall design (line-by-line nits are welcome if you're up to it, though!). Please let me know if you have any questions or suggestions. Thanks!
I don't know much about the video stack so I just have some more general questions: Why does the VideoFrameProvider have both a getCurrentFrame() and a putCurrentFrame()? I'm having trouble figuring out which way data is going here. I would guess from the name VideoFrameProvider that it's an object that gives you something, not that expects to get. Can getCurrentFrame() return a pointer instead of taking a **? When the texture rect is resized, is the old texture being deleted somehow? Declaring static const ints or size_ts in a .h and not providing storage for them in a .cpp is an error and won't work across all compilers we use. Either make these enums (ugly) or leave the values out of the .h and define them in a .cpp. People reading the header file shouldn't depend on particular values of the constants anyway so hiding them in a .cpp seems better to me. There aren't many uses of size_t in the WebKit API except for things that are numbers of bytes. Can the plane-related constants be unsigneds or ints?
Comment on attachment 63661 [details] Patch I mostly looked at the platform/graphics/chromium code as that the side I'm most familiar with.
Comment on attachment 63661 [details] Patch One more try as me previous comments got lost... Again, I mostly focused on the platform/graphics/chromium side of the code. I suspect that some of these issues you're already aware of as you said the code isn't complete yet, but just in case: WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:71 + requiredTextureSize = m_bounds; I believe the texture size should match your frame size rather than the bounds of the layer. The layer could be scaled independent of the video frame. WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:84 + m_provider->getCurrentFrame(&frame); Do you need to check the surface type and format before proceeding? WebCore/platform/graphics/chromium/VideoLayerChromium.cpp: + return; What happens is pixels == NULL ? WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:118 + void* mem = glMapTexSubImage2D(GL_TEXTURE_2D, 0, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height(), GL_LUMINANCE, GL_UNSIGNED_BYTE, GL_WRITE_ONLY); ASSERT(mem) just to be sure ?
Comment on attachment 63661 [details] Patch WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:435 + m_webMediaPlayer->putCurrentFrame(VideoFrameChromiumImpl::toWebVideoFrame(frame)); probably need a 'delete frame' here. should document the memory ownership model. i have further review comments about style, etc. will get to those ASAP. see also AssertMatchingEnums.cpp. i think you'll want to extend that for your new enums. constants should not have a 'k' prefix.
As I mentioned in person, sorry that the code is pretty rough! I didn't write my code with much care line-by-line as I wanted to get the overall design OKed by everyone first. Because of that, it'd probably be best to comment on overall design instead of on nits since I will have to break up the patch and submit it in pieces before I can commit it anyway, at which time I was planning on polishing up the details. An in-depth code review should make more sense/be less painful then, hopefully. :) Thanks so much! (In reply to comment #8) > (From update of attachment 63661 [details]) > WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:435 > + m_webMediaPlayer->putCurrentFrame(VideoFrameChromiumImpl::toWebVideoFrame(frame)); > probably need a 'delete frame' here. should document the memory ownership model. > > i have further review comments about style, etc. will get to those ASAP. > > see also AssertMatchingEnums.cpp. i think you'll want to extend that for your new enums. > > constants should not have a 'k' prefix.
Comment on attachment 63661 [details] Patch WebKit/chromium/src/VideoFrameChromiumImpl.cpp:13 + WebVideoFrame* VideoFrameChromiumImpl::toWebVideoFrame(WebCore::VideoFrameChromium* frame) the choice of variable names "frame" and "m_webFrame" is probably not so great since those are used when we have WebCore::Frame and WebKit::WebFrame pointers. WebKit/chromium/public/WebVideoFrame.h:41 + // WebKit::WebVideoFrame. it is fairly unfortunate that we need to replicate all of these constants. WebCore/platform/graphics/chromium/VideoFrameChromium.h:74 + virtual SurfaceType type() const = 0; i don't see any usage of type and format methods. is that an oversight? or just something that will happen later? WebKit/chromium/public/WebVideoFrame.h:75 + virtual SurfaceType type() const = 0; instead of exposing all of this information, have you considered just having a method on this interface that you can call to load the frame into a GLES2 texture? how much information about the frame do you really need to expose to WebKit? WebCore/platform/graphics/chromium/VideoFrameChromium.h:36 + class VideoFrameChromium { we typically avoid this double thunking by making the WebCore "interface" use non virtual methods. then, we just provide the concrete implementation in WebKit/chromium/src/, and that implementation then calls out to the WebKit API equivalent which provides the virtual functions.
(In reply to comment #10) > (From update of attachment 63661 [details]) > WebKit/chromium/src/VideoFrameChromiumImpl.cpp:13 > + WebVideoFrame* VideoFrameChromiumImpl::toWebVideoFrame(WebCore::VideoFrameChromium* frame) > the choice of variable names "frame" and "m_webFrame" is probably > not so great since those are used when we have WebCore::Frame > and WebKit::WebFrame pointers. > Good call, thanks! > WebCore/platform/graphics/chromium/VideoFrameChromium.h:74 > + virtual SurfaceType type() const = 0; > i don't see any usage of type and format methods. is that an oversight? > or just something that will happen later? > In the (near) future, I think it'd be nice to have access to these Format/SurfaceType methods. Right now the code just assumes that the format is YV12 and the surface type is system memory, but these are not good assumptions to be making. > WebKit/chromium/public/WebVideoFrame.h:75 > + virtual SurfaceType type() const = 0; > instead of exposing all of this information, have you considered > just having a method on this interface that you can call to load > the frame into a GLES2 texture? how much information about the > frame do you really need to expose to WebKit? > This is a good question. I did think about having a method like that, but the shader requires knowledge of the video frame such as dimensions and stride. So to completely push VideoFrame out of WebKit, the shader logic would have to be pushed out of WebKit as well, which doesn't seem very consistent with the rest of the design. But I will experiment; it is probably true that I don't need all this frame information exposed to WebKit and I should be able to strip it down to the bare minimum. The design is also going to morph a bit as I try to eliminate our extra memcopies. > WebCore/platform/graphics/chromium/VideoFrameChromium.h:36 > + class VideoFrameChromium { > we typically avoid this double thunking by making the WebCore "interface" > use non virtual methods. then, we just provide the concrete implementation > in WebKit/chromium/src/, and that implementation then calls out to the > WebKit API equivalent which provides the virtual functions. Perhaps I am misunderstanding (I am still a bit of a C++ novice), but I am not sure if I can do this because VideoFrameChromiumImpl is just a wrapper for WebKit::WebVideoFrame and the constructor needs to take a WebVideoFrame as a parameter. If I got rid of VideoFrameChromiumImpl.h, the VideoFrameChromium.h would need to have that parameter instead, which I believe is no good since it is mixing WebKit types in WebCore. Thanks so much for taking a look! I will update with a new patch pretty soon as I play with reducing memcopies.
(In reply to comment #11) > > WebCore/platform/graphics/chromium/VideoFrameChromium.h:36 > > + class VideoFrameChromium { > > we typically avoid this double thunking by making the WebCore "interface" > > use non virtual methods. then, we just provide the concrete implementation > > in WebKit/chromium/src/, and that implementation then calls out to the > > WebKit API equivalent which provides the virtual functions. > Perhaps I am misunderstanding (I am still a bit of a C++ novice), but I am not sure if I can do this because VideoFrameChromiumImpl is just a wrapper for WebKit::WebVideoFrame and the constructor needs to take a WebVideoFrame as a parameter. If I got rid of VideoFrameChromiumImpl.h, the VideoFrameChromium.h would need to have that parameter instead, which I believe is no good since it is mixing WebKit types in WebCore. Good point. I was just hoping to avoid some extra layers, but perhaps they are necessary.
My main concern with the existing patch is that it introduces a lot of redundancy. My suggestion to move the GL calls out to the embedder was just an effort to reduce the amount of fields that needed to be replicated between WebVideoFrame and VideoFrameChromium. Maybe there is a better way.
what's the status of this bug/patch?
Sorry for the confusion! This patch was a big monster that was not meant to be committed, but just meant to be discussed. The change was broken up into smaller changes that were committed here: http://trac.webkit.org/changeset/66071 http://src.chromium.org/viewvc/chrome?view=rev&revision=58022 And the third one's a-coming eventually! :) https://bugs.webkit.org/show_bug.cgi?id=45069