Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
128683003:
[PPAPI] Implement media stream video track API (Closed)
|
Created:
6 years, 11 months ago by Peng Modified:
6 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@video_track_impl_cl Visibility:
Public. |
Description[PPAPI] Implement media stream video track API
BUG=330851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245150
Patch Set 1 : Update #Patch Set 2 : Rebase it #
Total comments: 30
Patch Set 3 : Fix review issue #
Total comments: 54
Patch Set 4 : Fix review issues #
Total comments: 5
Patch Set 5 : Fix review issues #Patch Set 6 : Fix review issues #
Total comments: 11
Patch Set 7 : Fix review issues #
Total comments: 2
Patch Set 8 : Fix review issues #
Total comments: 18
Patch Set 9 : Fix review issues #
Total comments: 2
Patch Set 10 : Update #Patch Set 11 : Fix build errors. #Patch Set 12 : Fix build errors on Windows. #Patch Set 13 : Update #Messages
Total messages: 48 (0 generated)
Hi, This is the impl part of the media stream video track API. PTAL. Thanks. The all-in-one CL: https://codereview.chromium.org/101463008/
I may have more comments next go round, but I wanted you to get what I have so far. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_track_host_base.h:35: // Enqueue a frame into plugin's |frame_buffer_|. Can you clear up what this actually means? Does it mean that the host should call this when aframe is ready to pass to the plugin for reading? https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:102: for (size_t i = 0; i < VideoFrame::NumPlanes(frame->format()); ++i) { It's a shame we have to do this copy stuff. It would be nice if we had a way to integrate with the MediaStreamTrack/VideoFrame stuff to somehow say "Here's the memory you should use". Possible future work? https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.h:36: virtual void DidConnectPendingHostToResource() OVERRIDE; nit: I think all the virtual functions could be private. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.h:45: gfx::Size frame_size_; I found this name slightly confusing. It might be nice for the name to make the units clear and make it clear that it doesn't include the header size. So maybe... num_pixels_per_frame_ ? https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:112: new PpapiPluginMsg_MediaStreamVideoTrack_CreateFromPendingHost(id)); nit: should be indented 4 spaces https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:115: } Might be worth adding a TODO, if this is also where you plan to deal with Audio. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:167: blink::WebDOMMediaStreamTrack::fromV8Value(val); 4-space indent https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:171: scoped_ptr<IPC::Message> browser_host_create_message; It looks like this message is not necessary here; please delete. https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/plugin_var_t... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/plugin_var_t... ppapi/proxy/plugin_var_tracker.cc:209: MediaStreamVideoTrackResource *resource = It would be better to use scoped_refptr<> instead of a raw pointer. Probably does not matter in practice in your current code, but you're leaving a brief moment here where the object has 0 reference count. https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/ppapi_messag... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/ppapi_messag... ppapi/proxy/ppapi_messages.h:1418: std::string /* id */) Can you do a better name for this? Maybe "track_id" or something? (Same where you unpack the parameter) https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame.h:42: Audio audio; I think you need to add compile assertions to enforce the size of each type you put in the shared memory region. Everything you put in there is implicitly part of your IPC contract. In the case of NaCl, the plugin-side of the proxy is built using a different toolchain (always using the ILP32 ABI). This can lead to differences in size & alignment between plugin and renderer, for some structures. So you should assert here, document the reason. If they *don't* match, you may have to add some padding somewhere to force them to match. https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... File ppapi/shared_impl/media_stream_frame_buffer.h (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame_buffer.h:25: }; It might be simpler to just use a base::Closure for this? https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame_buffer.h:27: explicit MediaStreamFrameBuffer(Delegate* delegate); (If you keep the Delegate class, comment about ownership here... looks like it's unowned, and must outlive the MediaStreamFrameBuffer object). https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame_buffer.h:52: // A queue of incoming frame indexes. Can you elaborate more? It's not obvious what "incoming" means... incoming from the peer?
https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_track_host_base.h:35: // Enqueue a frame into plugin's |frame_buffer_|. On 2014/01/09 21:06:47, dmichael wrote: > Can you clear up what this actually means? Does it mean that the host should > call this when aframe is ready to pass to the plugin for reading? When a frame is ready, the host will call this function to send the frame index to the plugin, and then the plugin will add the index into a queue for reading. And I would like use this base class both for video input and output. For video input host, when a frame is consumed by host (rendered to <video> element), host will call this function to send the consumed frame index to the plugin, and then the plugin can reuse the frame for new frames. I think this function's name is not very good. Do you have any suggestion? https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:102: for (size_t i = 0; i < VideoFrame::NumPlanes(frame->format()); ++i) { On 2014/01/09 21:06:47, dmichael wrote: > It's a shame we have to do this copy stuff. It would be nice if we had a way to > integrate with the MediaStreamTrack/VideoFrame stuff to somehow say "Here's the > memory you should use". > > Possible future work? As my understanding, blink::MediaStreamTrack supports adding more than one sinks to it. So it has to create a frame and call every sinks' OnVideoFrame() with the same frame. :( https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.h:36: virtual void DidConnectPendingHostToResource() OVERRIDE; On 2014/01/09 21:06:47, dmichael wrote: > nit: I think all the virtual functions could be private. Done. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.h:45: gfx::Size frame_size_; On 2014/01/09 21:06:47, dmichael wrote: > I found this name slightly confusing. It might be nice for the name to make the > units clear and make it clear that it doesn't include the header size. So > maybe... > num_pixels_per_frame_ > ? No. It is a gfx::Size. I mean the width and height are in pixels. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:112: new PpapiPluginMsg_MediaStreamVideoTrack_CreateFromPendingHost(id)); On 2014/01/09 21:06:47, dmichael wrote: > nit: should be indented 4 spaces Done. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:115: } On 2014/01/09 21:06:47, dmichael wrote: > Might be worth adding a TODO, if this is also where you plan to deal with Audio. Done. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:167: blink::WebDOMMediaStreamTrack::fromV8Value(val); On 2014/01/09 21:06:47, dmichael wrote: > 4-space indent Done I don't know why vim continue giving me 2-space indent for this. Are you using some special vim setting for google code style? https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:171: scoped_ptr<IPC::Message> browser_host_create_message; On 2014/01/09 21:06:47, dmichael wrote: > It looks like this message is not necessary here; please delete. Done. https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/plugin_var_t... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/plugin_var_t... ppapi/proxy/plugin_var_tracker.cc:209: MediaStreamVideoTrackResource *resource = On 2014/01/09 21:06:47, dmichael wrote: > It would be better to use scoped_refptr<> instead of a raw pointer. Probably > does not matter in practice in your current code, but you're leaving a brief > moment here where the object has 0 reference count. I changed the code to call GetReference() at same line of new Resource(). Done. https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/ppapi_messag... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/proxy/ppapi_messag... ppapi/proxy/ppapi_messages.h:1418: std::string /* id */) On 2014/01/09 21:06:47, dmichael wrote: > Can you do a better name for this? Maybe "track_id" or something? (Same where > you unpack the parameter) Done. https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame.h:42: Audio audio; On 2014/01/09 21:06:47, dmichael wrote: > I think you need to add compile assertions to enforce the size of each type you > put in the shared memory region. Everything you put in there is implicitly part > of your IPC contract. In the case of NaCl, the plugin-side of the proxy is built > using a different toolchain (always using the ILP32 ABI). This can lead to > differences in size & alignment between plugin and renderer, for some > structures. So you should assert here, document the reason. If they *don't* > match, you may have to add some padding somewhere to force them to match. Done. https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... File ppapi/shared_impl/media_stream_frame_buffer.h (right): https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame_buffer.h:25: }; On 2014/01/09 21:06:47, dmichael wrote: > It might be simpler to just use a base::Closure for this? I tried. It does not simplify code too much. :( HostBase and ResourceBase will implement the Delegate and create MediaStreamFrameBuffer. But those two classes will not implement OnNewFrameEnqueued(), their subclasses may implement it.So use Closure is not very convenient. https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame_buffer.h:27: explicit MediaStreamFrameBuffer(Delegate* delegate); On 2014/01/09 21:06:47, dmichael wrote: > (If you keep the Delegate class, comment about ownership here... looks like > it's unowned, and must outlive the MediaStreamFrameBuffer object). Done. https://codereview.chromium.org/128683003/diff/60001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_frame_buffer.h:52: // A queue of incoming frame indexes. On 2014/01/09 21:06:47, dmichael wrote: > Can you elaborate more? It's not obvious what "incoming" means... incoming from > the peer? I added a class level document. Done
Thanks Peng. Mostly nits. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:20: case VideoFrame::YV12: wrong indent. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:83: // Drop frames if the underlying buffer is empty. underlying buffer in this case is actually 'full', right? https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:89: &(frame_buffer()->GetFramePointer(index)->video); wrong indent. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:18: public PepperMediaStreamTrackHostBase, wrong indent, and please also move ':' from the previous line to this line. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_track_resource_base.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_track_resource_base.cc:21: AttachToPendingHost(RENDERER, pending_renderer_id); wrong indent. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_track_resource_base.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_track_resource_base.h:34: // Enqueue a frame into host's |frame_queue_|. nit: Maybe say something like "Notifies the host to enqueue..."? https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:66: thunk::EnterResourceNoLock<thunk::PPB_VideoFrame_API>enter_frame(frame, true); one space before enter_frame, please. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:69: In another file, I mentioned that this class should maintain a list of unrecycled frames, in order to invalidate unrecycled frames on destruction. This method can also use that unrecycled frames list: we need to check whether |frame| is actually a frame returned by this object. Otherwise, the index may point to a buffer slot that is not owned by this object. And we mess up things. What do you think? https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:83: get_frame_callback_->PostRun(PP_ERROR_FAILED); It is more common to use PostAbort() in this case. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.h:17: public ppapi::thunk::PPB_MediaStreamVideoTrack_API { nit: ppapi:: is not needed. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:205: "Invalid message of type " tow more spaces indent, please. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... File ppapi/proxy/video_frame_resource.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... ppapi/proxy/video_frame_resource.cc:22: CHECK(!frame_) << "An unused (or unrecycled) frame is destroyed."; If I read the code correctly, currently the track resource doesn't keep a record of what VideoFrameResource objects are given to the plugin. When it dies, it doesn't notify all those unrecycled frames (if any) to invalidate. Therefore, those unrecycled frames carry dangling pointers and crash in the destructor. To me, it seems nicer to let the track resource keeps a record of those unrecycled frames, and invalidates them when the track dies. That way, even if the plugin is still holding references to those unrecycled frames, at least they don't have dangling pointers. What do you think? https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame.h:8: #include <string> nit: This is not needed? https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_frame_buffer.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:20: // maintian a queue of frames for reading or writing. nit: maint*ai*n https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:27: // 4. The reader receiveds the frame index and calls the reader's nit: receive*s* https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:33: // 7. The writer receives the frame index and put it back to the writer's free put*s* https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:39: public: Please consider adding a virtual destructor.
CL is updated. PTAL. Thanks. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:20: case VideoFrame::YV12: On 2014/01/10 21:05:17, yzshen1 wrote: > wrong indent. Done. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:83: // Drop frames if the underlying buffer is empty. On 2014/01/10 21:05:17, yzshen1 wrote: > underlying buffer in this case is actually 'full', right? Done https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:89: &(frame_buffer()->GetFramePointer(index)->video); On 2014/01/10 21:05:17, yzshen1 wrote: > wrong indent. Done. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:18: public PepperMediaStreamTrackHostBase, On 2014/01/10 21:05:17, yzshen1 wrote: > wrong indent, and please also move ':' from the previous line to this line. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_track_resource_base.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_track_resource_base.cc:21: AttachToPendingHost(RENDERER, pending_renderer_id); On 2014/01/10 21:05:17, yzshen1 wrote: > wrong indent. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_track_resource_base.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_track_resource_base.h:34: // Enqueue a frame into host's |frame_queue_|. On 2014/01/10 21:05:17, yzshen1 wrote: > nit: Maybe say something like "Notifies the host to enqueue..."? That is better. Done https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:66: thunk::EnterResourceNoLock<thunk::PPB_VideoFrame_API>enter_frame(frame, true); On 2014/01/10 21:05:17, yzshen1 wrote: > one space before enter_frame, please. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:69: On 2014/01/10 21:05:17, yzshen1 wrote: > In another file, I mentioned that this class should maintain a list of > unrecycled frames, in order to invalidate unrecycled frames on destruction. > > This method can also use that unrecycled frames list: we need to check whether > |frame| is actually a frame returned by this object. Otherwise, the index may > point to a buffer slot that is not owned by this object. And we mess up things. > > What do you think? It is a good idea. Done https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:83: get_frame_callback_->PostRun(PP_ERROR_FAILED); On 2014/01/10 21:05:17, yzshen1 wrote: > It is more common to use PostAbort() in this case. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.h:17: public ppapi::thunk::PPB_MediaStreamVideoTrack_API { On 2014/01/10 21:05:17, yzshen1 wrote: > nit: ppapi:: is not needed. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/plugin_var_... File ppapi/proxy/plugin_var_tracker.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/plugin_var_... ppapi/proxy/plugin_var_tracker.cc:205: "Invalid message of type " On 2014/01/10 21:05:17, yzshen1 wrote: > tow more spaces indent, please. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... File ppapi/proxy/video_frame_resource.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... ppapi/proxy/video_frame_resource.cc:22: CHECK(!frame_) << "An unused (or unrecycled) frame is destroyed."; On 2014/01/10 21:05:17, yzshen1 wrote: > If I read the code correctly, currently the track resource doesn't keep a record > of what VideoFrameResource objects are given to the plugin. When it dies, it > doesn't notify all those unrecycled frames (if any) to invalidate. > Therefore, those unrecycled frames carry dangling pointers and crash in the > destructor. > > To me, it seems nicer to let the track resource keeps a record of those > unrecycled frames, and invalidates them when the track dies. That way, even if > the plugin is still holding references to those unrecycled frames, at least they > don't have dangling pointers. > > What do you think? It is good idea. https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame.h:8: #include <string> On 2014/01/10 21:05:17, yzshen1 wrote: > nit: This is not needed? Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_frame_buffer.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:20: // maintian a queue of frames for reading or writing. On 2014/01/10 21:05:17, yzshen1 wrote: > nit: maint*ai*n Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:27: // 4. The reader receiveds the frame index and calls the reader's On 2014/01/10 21:05:17, yzshen1 wrote: > nit: receive*s* Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:33: // 7. The writer receives the frame index and put it back to the writer's free On 2014/01/10 21:05:17, yzshen1 wrote: > put*s* Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_frame_buffer.h:39: public: On 2014/01/10 21:05:17, yzshen1 wrote: > Please consider adding a virtual destructor. Done.
https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:32: // |frame_buffer_| for reading or writing. Also see |MediaStreamFrameBuffer|. Consider: // Sends a frame index to the corresponding MediaStreamTrackResourceBase // via an IPC message. The resource adds the frame index into its // |frame_buffer_| for reading or writing. Also see |MediaStreamFrameBuffer|. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:33: void PluginEnqueueFrame(int32_t index); Consider renaming this to SendEnqueueFrameMessageToPlugin. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:36: // being closed. Consider: // Subclasses must implement this method to clean up when the track is closed. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:103: for (size_t i = 0; i < VideoFrame::NumPlanes(frame->format()); ++i) { It might be slightly clearer and faster to assign this to a local variable, 'num_planes'. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:36: virtual void DidConnectPendingHostToResource() OVERRIDE; Could you comment this: // ResourceHost overrides. https://codereview.chromium.org/128683003/diff/600002/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/host/ppapi_host.h... ppapi/host/ppapi_host.h:66: //Similar to |SendUnsolicitedReply()|, but it supports sending file handles. Consider: // Similar to |SendUnsolicitedReply()|, but also sends handles. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_track_resource_base.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_track_resource_base.h:35: void HostEnqueueFrame(int32_t index); Consider renaming this to match the host method: SendEnqueueFrameMessageToHost https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:71: static_cast<VideoFrameResource*>(enter_frame.resource()); It's unusual to cast to the underlying type when we have an API layer. Could we add the methods you need to PPB_VideoFrame_API? I searched Chrome with this query: EnterResourceNoLock static_cast ".resource()" https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.h:28: ) OVERRIDE; It looks strange to split the ( and ) like this. How about: virtual thunk::PPB_MediaStreamVideoTrack_API* AsPPB_MediaStreamVideoTrack_API( ) OVERRIDE; https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... ppapi/proxy/video_frame_resource.h:37: friend class MediaStreamVideoTrackResource; If you add the needed methods to the API, you won't need this friend.
Mostly looks good. Thanks! https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:83: PpapiGlobals::Get()->GetResourceTracker()->ReleaseResource( I see that you take one ref to the keys of |frames_| (which are PP_Resources), you don't have to do that. A PP_Resource is assigned to a VideoFrameResource object, and it won't change or be assigned to others during the lifetime of VideoFrameResource. So you can use the PP_Resource as the key without holding a ref to it. https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:122: resource->GetReference(), resource)); The indent of this line is a bit weird.
https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:83: PpapiGlobals::Get()->GetResourceTracker()->ReleaseResource( On 2014/01/10 23:59:43, yzshen1 wrote: > I see that you take one ref to the keys of |frames_| (which are PP_Resources), > you don't have to do that. A PP_Resource is assigned to a VideoFrameResource > object, and it won't change or be assigned to others during the lifetime of > VideoFrameResource. So you can use the PP_Resource as the key without holding a > ref to it. I though holding a ref in frames_ can avoid the VideoFrameResource being destroyed. Is it necessary? Is holding scoped_refptr of the VideoFrameResource in |frames_| enough?
On 2014/01/11 00:30:55, Peng wrote: > https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... > File ppapi/proxy/media_stream_video_track_resource.cc (right): > > https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... > ppapi/proxy/media_stream_video_track_resource.cc:83: > PpapiGlobals::Get()->GetResourceTracker()->ReleaseResource( > On 2014/01/10 23:59:43, yzshen1 wrote: > > I see that you take one ref to the keys of |frames_| (which are PP_Resources), > > you don't have to do that. A PP_Resource is assigned to a VideoFrameResource > > object, and it won't change or be assigned to others during the lifetime of > > VideoFrameResource. So you can use the PP_Resource as the key without holding > a > > ref to it. > > I though holding a ref in frames_ can avoid the VideoFrameResource being > destroyed. Is it necessary? Is holding scoped_refptr of the VideoFrameResource > in |frames_| enough? Yes, that is enough.
CL is updated. PTAL. Thanks. https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:83: PpapiGlobals::Get()->GetResourceTracker()->ReleaseResource( On 2014/01/11 00:30:55, Peng wrote: > On 2014/01/10 23:59:43, yzshen1 wrote: > > I see that you take one ref to the keys of |frames_| (which are PP_Resources), > > you don't have to do that. A PP_Resource is assigned to a VideoFrameResource > > object, and it won't change or be assigned to others during the lifetime of > > VideoFrameResource. So you can use the PP_Resource as the key without holding > a > > ref to it. > > I though holding a ref in frames_ can avoid the VideoFrameResource being > destroyed. Is it necessary? Is holding scoped_refptr of the VideoFrameResource > in |frames_| enough? > Done. https://codereview.chromium.org/128683003/diff/750001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:122: resource->GetReference(), resource)); On 2014/01/10 23:59:43, yzshen1 wrote: > The indent of this line is a bit weird. Done.
LGTM Thanks for the nice work!
On 2014/01/13 05:18:55, yzshen1 wrote: > LGTM > > Thanks for the nice work! Hi Peng, I had a round of comments on patchset 3 that got lost I think.
CL is updated. Please take a look. Thanks. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:32: // |frame_buffer_| for reading or writing. Also see |MediaStreamFrameBuffer|. On 2014/01/10 22:57:06, bbudge wrote: > Consider: > // Sends a frame index to the corresponding MediaStreamTrackResourceBase > // via an IPC message. The resource adds the frame index into its > // |frame_buffer_| for reading or writing. Also see |MediaStreamFrameBuffer|. Done. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:33: void PluginEnqueueFrame(int32_t index); On 2014/01/10 22:57:06, bbudge wrote: > Consider renaming this to SendEnqueueFrameMessageToPlugin. Done. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:36: // being closed. On 2014/01/10 22:57:06, bbudge wrote: > Consider: > // Subclasses must implement this method to clean up when the track is closed. Done. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:103: for (size_t i = 0; i < VideoFrame::NumPlanes(frame->format()); ++i) { On 2014/01/10 22:57:06, bbudge wrote: > It might be slightly clearer and faster to assign this to a local variable, > 'num_planes'. Done. https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/128683003/diff/600002/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:36: virtual void DidConnectPendingHostToResource() OVERRIDE; On 2014/01/10 22:57:06, bbudge wrote: > Could you comment this: > // ResourceHost overrides. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/host/ppapi_host.h... ppapi/host/ppapi_host.h:66: //Similar to |SendUnsolicitedReply()|, but it supports sending file handles. On 2014/01/10 22:57:06, bbudge wrote: > Consider: > // Similar to |SendUnsolicitedReply()|, but also sends handles. Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_track_resource_base.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_track_resource_base.h:35: void HostEnqueueFrame(int32_t index); On 2014/01/10 22:57:06, bbudge wrote: > Consider renaming this to match the host method: > SendEnqueueFrameMessageToHost Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:71: static_cast<VideoFrameResource*>(enter_frame.resource()); On 2014/01/10 22:57:06, bbudge wrote: > It's unusual to cast to the underlying type when we have an API layer. Could we > add the methods you need to PPB_VideoFrame_API? > > I searched Chrome with this query: > EnterResourceNoLock static_cast ".resource()" As yzshen's suggestion, I use a FrameMap here to convert a PP_Resource to FrameResource pointer. I think it resolve this issue. Done https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.h:28: ) OVERRIDE; On 2014/01/10 22:57:06, bbudge wrote: > It looks strange to split the ( and ) like this. How about: > > virtual thunk::PPB_MediaStreamVideoTrack_API* > AsPPB_MediaStreamVideoTrack_API( ) OVERRIDE; Done. https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/600002/ppapi/proxy/video_frame... ppapi/proxy/video_frame_resource.h:37: friend class MediaStreamVideoTrackResource; On 2014/01/10 22:57:06, bbudge wrote: > If you add the needed methods to the API, you won't need this friend. MediaStreamVideoTrackResource only needs below four private methods. Using friend class is to make sure other classes can not use them. Anyway. change them to public methods. Done
Hi palmer, Could you please review changes in content/content_renderer.gypi & ppapi/proxy/ppapi_messages.h? Thanks.
https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... ppapi/proxy/video_frame_resource.h:33: } Sorry if I wasn't very clear before. I was thinking that you could make these methods part of the PPB_VideoFrame_API. Thunk APIs can contain stuff that is only needed by internal proxy code - they don't have to be in 1:1 correspondence with the Pepper interface. If you don't want to do that, then I would prefer that these be private, with the video track class being a friend. Like you had before.
https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... ppapi/proxy/video_frame_resource.h:33: } Oops! I misunderstood your comment. I would like the constructor can only be accessed by MediaStreamVideoTrackResource as well. I think it can not be added into PPB_VideoFrame_API, so probably using friend class is better. Change it back. Done. On 2014/01/13 20:03:52, bbudge wrote: > Sorry if I wasn't very clear before. I was thinking that you could make these > methods part of the PPB_VideoFrame_API. Thunk APIs can contain stuff that is > only needed by internal proxy code - they don't have to be in 1:1 correspondence > with the Pepper interface. > > If you don't want to do that, then I would prefer that these be private, with > the video track class being a friend. Like you had before.
https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... ppapi/proxy/video_frame_resource.h:33: } On 2014/01/13 20:26:55, Peng wrote: > Oops! I misunderstood your comment. > > I would like the constructor can only be accessed by > MediaStreamVideoTrackResource as well. I think it can not be added into > PPB_VideoFrame_API, so probably using friend class is better. Change it back. > Done. > > > On 2014/01/13 20:03:52, bbudge wrote: > > Sorry if I wasn't very clear before. I was thinking that you could make these > > methods part of the PPB_VideoFrame_API. Thunk APIs can contain stuff that is > > only needed by internal proxy code - they don't have to be in 1:1 > correspondence > > with the Pepper interface. > > > > If you don't want to do that, then I would prefer that these be private, with > > the video track class being a friend. Like you had before. > Is it that important that the constructor be private? I realize that you only want these to be created by the MediaStreamVideoTrackResource class, but I don't think it would be a hazard for anyone working on proxy code if the constructor was public. Maybe I'm missing something though. The reason I'm pushing on this is that we already have a way to express what the internal API for a resource class is - by defining one of these APIs. This would be the first case in ppapi/proxy where 'friend' is used to give a resource access into another resource. All other usages of 'friend' in resources are either for ref counting or test code. There are several cases of classes interacting through the API interface classes, e.g. FileIOResource and FileSystemResource. It does add another file to your patch but I think adding these methods to PPB_VideoFrame_API will make the code more like the rest of the proxy and so a little easier to understand.
https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.h:45: gfx::Size frame_size_; On 2014/01/10 19:14:30, Peng wrote: > On 2014/01/09 21:06:47, dmichael wrote: > > I found this name slightly confusing. It might be nice for the name to make > the > > units clear and make it clear that it doesn't include the header size. So > > maybe... > > num_pixels_per_frame_ > > ? > > No. It is a gfx::Size. I mean the width and height are in pixels. Okay, sorry for my misunderstanding. I think the previous comment was better, or no comment at all. The current comment just rephrases the variable name. https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/resource_converter.cc (right): https://codereview.chromium.org/128683003/diff/60001/content/renderer/pepper/... content/renderer/pepper/resource_converter.cc:167: blink::WebDOMMediaStreamTrack::fromV8Value(val); On 2014/01/10 19:14:30, Peng wrote: > On 2014/01/09 21:06:47, dmichael wrote: > > 4-space indent > > Done > > I don't know why vim continue giving me 2-space indent for this. Are you using > some special vim setting for google code style? No... when I first started, I was using the recommended VIM style customizations, but they got in my way too much. Now I do it all manually :-/ Best approach these days is probably to do whatever is easiest for you in vim, not worry too much about style, but use the clang formatter. https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_track_host_base.h:33: void SendEnqueueFrameMessageToPlugin(int32_t index); Thanks, this is better. Another possibility might be: s/EnqueueFrame/FrameReady https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_track_host_base.h:36: virtual void OnClose() = 0; (Without checking, I would expect OnClose to come from the base class, so it could maybe be private.) https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/media_stre... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:68: return PP_ERROR_FAILED; This seems a little strange to me. I think you're trying to account for the fact that when the track is closed, you invalidate all the frames and clear your map. But why should the plugin have to care? It seems like we could happily just say "PP_OK" if they try to recycle a frame after end/close. One way to accomplish that would be to *not* clear the map, but only invalidate all the resources it refers to. You could even release the pointer and leave the PP_Resource in the map with a null pointer value. What do you think? On a related note... you appear not to ever look for an end_of_stream frame. So maybe you actually need two distinct states for this: closed and ended... I'm not sure if there are noticeable problems with the way you have it, but it's a little confusing that has_ended doesn't necessarily mean that we reached the end of the track. https://codereview.chromium.org/128683003/diff/1380002/ppapi/shared_impl/medi... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/1380002/ppapi/shared_impl/medi... ppapi/shared_impl/media_stream_frame.h:37: uint8_t data[0]; zero-sized arrays are non-standard, I think. This might run you in to trouble with one or more of our compilers. You might have to pad the struct, instead.
CL is updated. Please take a look. Thanks. https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_track_host_base.h:33: void SendEnqueueFrameMessageToPlugin(int32_t index); On 2014/01/13 21:26:47, dmichael wrote: > Thanks, this is better. Another possibility might be: > s/EnqueueFrame/FrameReady In this CL, it is used for sending a ready frame to plugin. But it can also be used for sending consumed frame (recycled frame) in VideoProducer CL. So I think maybe EnqueueFrame is better. Thought? https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_track_host_base.h:36: virtual void OnClose() = 0; On 2014/01/13 21:26:47, dmichael wrote: > (Without checking, I would expect OnClose to come from the base class, so it > could maybe be private.) Done. https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/media_stre... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:68: return PP_ERROR_FAILED; On 2014/01/13 21:26:47, dmichael wrote: > This seems a little strange to me. I think you're trying to account for the fact > that when the track is closed, you invalidate all the frames and clear your map. > But why should the plugin have to care? It seems like we could happily just say > "PP_OK" if they try to recycle a frame after end/close. One way to accomplish > that would be to *not* clear the map, but only invalidate all the resources it > refers to. You could even release the pointer and leave the PP_Resource in the > map with a null pointer value. What do you think? Done. > > On a related note... you appear not to ever look for an end_of_stream frame. So > maybe you actually need two distinct states for this: closed and ended... I'm > not sure if there are noticeable problems with the way you have it, but it's a > little confusing that has_ended doesn't necessarily mean that we reached the end > of the track. Add a TODO in Host. will support close the track from host side. https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/1260001/ppapi/proxy/video_fram... ppapi/proxy/video_frame_resource.h:33: } On 2014/01/13 20:52:44, bbudge wrote: > On 2014/01/13 20:26:55, Peng wrote: > > Oops! I misunderstood your comment. > > > > I would like the constructor can only be accessed by > > MediaStreamVideoTrackResource as well. I think it can not be added into > > PPB_VideoFrame_API, so probably using friend class is better. Change it back. > > Done. > > > > > > On 2014/01/13 20:03:52, bbudge wrote: > > > Sorry if I wasn't very clear before. I was thinking that you could make > these > > > methods part of the PPB_VideoFrame_API. Thunk APIs can contain stuff that is > > > only needed by internal proxy code - they don't have to be in 1:1 > > correspondence > > > with the Pepper interface. > > > > > > If you don't want to do that, then I would prefer that these be private, > with > > > the video track class being a friend. Like you had before. > > > > Is it that important that the constructor be private? I realize that you only > want these to be created by the MediaStreamVideoTrackResource class, but I don't > think it would be a hazard for anyone working on proxy code if the constructor > was public. Maybe I'm missing something though. > > The reason I'm pushing on this is that we already have a way to express what the > internal API for a resource class is - by defining one of these APIs. This would > be the first case in ppapi/proxy where 'friend' is used to give a resource > access into another resource. All other usages of 'friend' in resources are > either for ref counting or test code. There are several cases of classes > interacting through the API interface classes, e.g. FileIOResource and > FileSystemResource. > > It does add another file to your patch but I think adding these methods to > PPB_VideoFrame_API will make the code more like the rest of the proxy and so a > little easier to understand. Done. https://codereview.chromium.org/128683003/diff/1380002/ppapi/shared_impl/medi... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/1380002/ppapi/shared_impl/medi... ppapi/shared_impl/media_stream_frame.h:37: uint8_t data[0]; On 2014/01/13 21:26:47, dmichael wrote: > zero-sized arrays are non-standard, I think. This might run you in to trouble > with one or more of our compilers. You might have to pad the struct, instead. Done.
Just a couple comments on comments. Nice work, lgtm I'm assuming a test CL is coming soon? Thanks again. https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/1260001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_track_host_base.h:33: void SendEnqueueFrameMessageToPlugin(int32_t index); On 2014/01/14 15:55:55, Peng wrote: > On 2014/01/13 21:26:47, dmichael wrote: > > Thanks, this is better. Another possibility might be: > > s/EnqueueFrame/FrameReady > > In this CL, it is used for sending a ready frame to plugin. But it can also be > used for sending consumed frame (recycled frame) in VideoProducer CL. So I think > maybe EnqueueFrame is better. Thought? > Sure, makes sense. I think the longer name you're using now is good as-is. https://codereview.chromium.org/128683003/diff/1540001/ppapi/shared_impl/medi... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/shared_impl/medi... ppapi/shared_impl/media_stream_frame.h:23: PP_COMPILE_ASSERT_SIZE_IN_BYTES(Header, 8); It would be helpful to readers to comment on why it's important to have consistent size... E.g.: """ Because these structs are written and read in shared memory, we need the size and alighment to be consistent between NaCl and its host trusted platform. """ https://codereview.chromium.org/128683003/diff/1540001/ppapi/shared_impl/medi... ppapi/shared_impl/media_stream_frame.h:37: // Uses 8 bytes to make sure below compile assert work in all platforms. I would maybe rephrase to: "Uses 8 bytes to make sure the Video struct has consistent size between NaCl code and renderer code."
https://codereview.chromium.org/128683003/diff/1540001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/1540001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_track_host_base.h:19: public ppapi::MediaStreamFrameBuffer::Delegate { I can't find any override of the Delegate::OnNewFrameEnqueued method on this class or on the subclass. Is there a follow-up patch that will define that?
https://codereview.chromium.org/128683003/diff/1540001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/128683003/diff/1540001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_track_host_base.h:19: public ppapi::MediaStreamFrameBuffer::Delegate { On 2014/01/14 17:13:52, bbudge wrote: > I can't find any override of the Delegate::OnNewFrameEnqueued method on this > class or on the subclass. Is there a follow-up patch that will define that? Yes. I plan to use this class as a base class for both A/V input and output. For A/V output(the 'output' is from plugin's point of view), subclasses will implement OnNewFrameEnqueued() to deliver frames to blink::MediaStreamTrack for rendering, etc.
https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:104: get_frame_callback_->PostRun(PP_OK); This is called by a message handler, plugin code is not on the stack. Therefore, I think it is sufficient to Run() instead of PostRun(), we won't reenter the plugin code. But we do need to clean |get_frame_callback_| and |get_frame_output_| first in case the plugin reenters us. (On the contrary, the PostAbort() on line 92 is necessary, using Abort() may re-enter the plugin code.) https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:117: frames_.insert(FrameMap::value_type(resource->GetReference(), resource)); The current code looks like |frames_| owns a ref to the PP_Resource (because of the GetReference() call), and the return value doesn't (because of pp_resource()). It seems better to add comment saying that the ref is returned to the plugin instead of owned by |frames_|.
https://codereview.chromium.org/128683003/diff/1540001/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/host/ppapi_host.... ppapi/host/ppapi_host.h:66: //Similar to |SendUnsolicitedReply()|, but also sends handles. space after // https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:77: thunk::EnterResourceNoLock<thunk::PPB_VideoFrame_API> enter(frame, false); You can avoid the EnterResource stuff and reference the VideoFrameResource directly, via the scoped_refptr. You'd have to make all of the PPB_VideoFrame_API methods public on the class. That shouldn't be a problem. The thunk API stuff was needed before we switched to the new resource system. The APIs were needed because there were multiple implementors. Now, only the resource class implements the API, so it's mainly useful to show what methods are intended to be callable by the internal proxy code. The API doesn't need to correspond to the resource API that the plugin sees. https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:123: while (it != frames_.end()) { Again, just use the VideoFrameResource directly via the scoped_refptr now, i.e. it->second->Invalidate(). https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/video_fram... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/video_fram... ppapi/proxy/video_frame_resource.h:39: // PPB_VideoFrame_API private API overrides: I would just make these public and group them above.
CL is updated. PTAL. Thanks. https://codereview.chromium.org/128683003/diff/1540001/ppapi/host/ppapi_host.h File ppapi/host/ppapi_host.h (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/host/ppapi_host.... ppapi/host/ppapi_host.h:66: //Similar to |SendUnsolicitedReply()|, but also sends handles. On 2014/01/14 20:52:11, bbudge wrote: > space after // Done. https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:77: thunk::EnterResourceNoLock<thunk::PPB_VideoFrame_API> enter(frame, false); Done. I like use scoped_refptr<VideoFrameResource> directly. :) On 2014/01/14 20:52:11, bbudge wrote: > You can avoid the EnterResource stuff and reference the VideoFrameResource > directly, via the scoped_refptr. You'd have to make all of the > PPB_VideoFrame_API methods public on the class. > > That shouldn't be a problem. The thunk API stuff was needed before we switched > to the new resource system. The APIs were needed because there were multiple > implementors. Now, only the resource class implements the API, so it's mainly > useful to show what methods are intended to be callable by the internal proxy > code. The API doesn't need to correspond to the resource API that the plugin > sees. https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:104: get_frame_callback_->PostRun(PP_OK); On 2014/01/14 20:47:29, yzshen1 wrote: > This is called by a message handler, plugin code is not on the stack. Therefore, > I think it is sufficient to Run() instead of PostRun(), we won't reenter the > plugin code. But we do need to clean |get_frame_callback_| and > |get_frame_output_| first in case the plugin reenters us. > > (On the contrary, the PostAbort() on line 92 is necessary, using Abort() may > re-enter the plugin code.) Thanks for detailed explanation. Now understand when need PostRun(). Done. https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:117: frames_.insert(FrameMap::value_type(resource->GetReference(), resource)); On 2014/01/14 20:47:29, yzshen1 wrote: > The current code looks like |frames_| owns a ref to the PP_Resource (because of > the GetReference() call), and the return value doesn't (because of > pp_resource()). > > It seems better to add comment saying that the ref is returned to the plugin > instead of owned by |frames_|. I changed the code to use pp_resource() at map.insert() and GetReference() at return. Done https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/media_stre... ppapi/proxy/media_stream_video_track_resource.cc:123: while (it != frames_.end()) { On 2014/01/14 20:52:11, bbudge wrote: > Again, just use the VideoFrameResource directly via the scoped_refptr now, i.e. > it->second->Invalidate(). Done. https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/video_fram... File ppapi/proxy/video_frame_resource.h (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/proxy/video_fram... ppapi/proxy/video_frame_resource.h:39: // PPB_VideoFrame_API private API overrides: On 2014/01/14 20:52:11, bbudge wrote: > I would just make these public and group them above. Done. https://codereview.chromium.org/128683003/diff/1540001/ppapi/shared_impl/medi... File ppapi/shared_impl/media_stream_frame.h (right): https://codereview.chromium.org/128683003/diff/1540001/ppapi/shared_impl/medi... ppapi/shared_impl/media_stream_frame.h:23: PP_COMPILE_ASSERT_SIZE_IN_BYTES(Header, 8); On 2014/01/14 16:12:03, dmichael wrote: > It would be helpful to readers to comment on why it's important to have > consistent size... E.g.: > """ > Because these structs are written and read in shared memory, we need the size > and alighment to be consistent between NaCl and its host trusted platform. > """ Done. https://codereview.chromium.org/128683003/diff/1540001/ppapi/shared_impl/medi... ppapi/shared_impl/media_stream_frame.h:37: // Uses 8 bytes to make sure below compile assert work in all platforms. On 2014/01/14 16:12:03, dmichael wrote: > I would maybe rephrase to: "Uses 8 bytes to make sure the Video struct has > consistent size between NaCl code and renderer code." Done.
Thanks for review. I will create some tests in separate CL. On 2014/01/14 16:12:02, dmichael wrote: > Just a couple comments on comments. Nice work, lgtm > > I'm assuming a test CL is coming soon? > > Thanks again.
LGTM Thanks for doing this!
still LGTM Thanks!
thanks Yuzhu & Bill
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/128683003/1850001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/128683003/1850001
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Hi James, Could you please review changes in content/content_renderer.gypi. Thanks.
Hi Thomas, could you please review changes in ppapi/proxy/ppapi_messages.h? Thanks.
Messages LGTM. https://codereview.chromium.org/128683003/diff/1850001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/128683003/diff/1850001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:1416: // Message for init frames. It also takes a shared memory handle. nit: I'm not sure I understand your comment about also taking a shared memory handle.
https://codereview.chromium.org/128683003/diff/1850001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/128683003/diff/1850001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:1416: // Message for init frames. It also takes a shared memory handle. On 2014/01/15 17:51:47, Tom Sepez wrote: > nit: I'm not sure I understand your comment about also taking a shared memory > handle. This message is sent from a Pepper host to a pepper resource. It will be embedded into a PpapiPluginMsg_ResourceReply message which takes the shared memory handle.
please TBR file list updates in gypi files, they don't need a full review. lgtm for content_renderer.gypi
On 2014/01/15 18:53:32, jamesr wrote: > please TBR file list updates in gypi files, they don't need a full review. > > lgtm for content_renderer.gypi I see. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/128683003/1850001
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, base_unittests_swarm, browser_tests, browser_tests_swarm, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, interactive_ui_tests_swarm, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, net_unittests, net_unittests_swarm, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests, unit_tests_swarm, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/128683003/80002
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/128683003/2730001
Failed to trigger a try job on win_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/128683003/2910001
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/128683003/2220003
Message was sent while issue was closed.
Change committed as 245150 |
