Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
107083004:
[PPAPI] API definition for video media stream artifacts (Closed)
|
Created:
7 years ago by Peng Modified:
6 years, 11 months ago Reviewers:
bbudge, noelallen_use_chromium, dmichael (off chromium), teravest, yzshen1, Sam Clegg, binji CC:
chromium-reviews, yusukes+watch_chromium.org, raymes+watch_chromium.org, yzshen+watch_chromium.org, teravest+watch_chromium.org, Sam Clegg, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
Description[PPAPI] API definition for video media stream artifacts
This API follows the design at
https://docs.google.com/a/google.com/document/d/1rlwmFhf7VCX8mfrBok8wqXNgvr_ERhL2k6Fqha-pgIo/edit?disco=AAAAAHos8Y8#
It defines new objects for the consumption of media video tracks based on the private VideoSource/VideoDestination classes.
BUG=330851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243099
Patch Set 1 #Patch Set 2 : Update #
Total comments: 50
Patch Set 3 : Update #
Total comments: 54
Patch Set 4 : Update #Patch Set 5 : update #
Total comments: 14
Patch Set 6 : Update #
Total comments: 6
Patch Set 7 : Update #Patch Set 8 : Update #
Total comments: 10
Patch Set 9 : Update #
Total comments: 32
Patch Set 10 : Update #Patch Set 11 : Fix build warnings #
Messages
Total messages: 33 (0 generated)
Hi, I took over Greg's CL 53593003, and modified it base on our discussion. PTAL. Thanks.
Overall, I think I like the shared memory buffer + Resource approach. (I haven't looked carefully at the C++ yet.) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:8: * receiving video frames from a MediaStream video track in the browser. What's the plan for pushing frames to a track? Will that eventually go in this API, or will that be a separate one? I ask mainly because the name you have here seems to imply it will do both (or it will be hard to name the one for pushing frames). https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:27: PP_Bool IsMediaStreamVideoTrack([in]PP_Resource resource); style/consistency nit: please put a space between [in] and the parameter type (here and elsewhere) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:30: * Configure underlayer frame buffers for incoming frames. The provided underlayer->underlying? https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:39: int32_t Configure([in]PP_Resource resource, [in]uint32_t frame_buffer_size); Here and in methods other than "IsMediaStreamVideoTrack", we would usually use a more specific name than |resource| to imply that it must be a MediaStreamVideoTrack resource. "video_track" is probably sufficient. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:78: * Returns PP_ERROR_BADRESOURCE if resource isn't a valid video source. (I don't think you need to list PP_ERROR_BADRESOURCE explicitly) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:82: * browser error occurs. (Ditto for PP_ERROR_FAILED; these are pretty common and used consistently, so referencing pp_errors.h is sufficient. Same applies to other places you list these two.) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:90: * new frames.Gets the next video frame from the MediaStream track. I don't think you want the last sentence here... Also, it would be good to mention that the given frame is invalid after this call. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:104: int32_t ReuseFrame([in] PP_Resource resource, I think we might be able to come up with a better name. "ReuseFrame" seems to imply the frame is going to get used for something as a result of this call, when really it's just telling the track that we're done with it. One possible alternative is "RecycleFrame". https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:42: PP_VIDEOFRAME_FORMAT_YV12J = 7 Why is this 7? Are all of these uncompressed, and so a frame has fixed size? (pardon my ignorance) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:66: void SetTimestamp([in] PP_Resource resource, [in] PP_TimeDelta timestamp); Hmm, you know, any Pepper calls we make require acquiring the proxy lock, which might be heavily contested. Would it make sense at all to instead make the timestamp a parameter to the PutFrame function (or whatever it will be called)?
Comments for IDLs. Will look at other files. Thanks! https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:12: M33 = 0.1 I think we can now use the [channel=dev] tag, because Justin has landed the support: https://code.google.com/p/chromium/issues/detail?id=325403 https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:30: * Configure underlayer frame buffers for incoming frames. The provided - Configure*s* - underlayer -> underlying? https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:31: * |frame_buffer_size| is provided in terms of the number of frames to use for Other methods use @param[in] @return tag, please make them consistent. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:34: * inter-frame processing time variability won't overrun the ring buffer. Nit: no need to mention 'ring' buffer. That is an implementation detail. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:49: PP_Var GetId([in]PP_Resource resource); Except the IsMediaStreamVideoTrack() method, please use |media_stream_video_track| as parameter name instead of |resource|. (here and below) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:68: * another frame to be buffered. It seems nice to also comment that once the input buffer is full, further GetFrame() calls won't complete until ReuseFrame() is called to free up buffer slots. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:89: * Release a frame got from |GetFrame()|, so the track can reuse it for Release*s* https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:90: * new frames.Gets the next video frame from the MediaStream track. - One space before "Gets", please. - Please talk about "the application shouldn't use |frame| anymore and should release references it holds to |frame|. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:108: * Closes the MediaStream video track. Please talk about whether it is allowed to do Configure() / GetFrame() / etc. after Close(). (I think we probably don't need to support re-Configure(), right?) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:42: PP_VIDEOFRAME_FORMAT_YV12J = 7 I can see that you want to match the enum values of media/base/video_frame.h. But maybe it is better to not leave holes and do explicit conversion? https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:57: * Get timestamp of the video frame. Given in seconds since the Get*s* (Here and similar issues below.) https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:58: * start of the containing video stream. You could use @param[in] @return to be consistent. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:60: PP_TimeDelta GetTimestamp([in] PP_Resource resource); Please change all |resource| below to |video_frame|.
https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:8: * receiving video frames from a MediaStream video track in the browser. On 2013/12/20 17:42:52, dmichael wrote: > What's the plan for pushing frames to a track? Will that eventually go in this > API, or will that be a separate one? I ask mainly because the name you have here > seems to imply it will do both (or it will be hard to name the one for pushing > frames). For pushing frames, we will have a separate one. How about using MediaStreamVideoSource / MediaStreamVideoTrackSource? I feel MSVTS is too long. :( And if we use *VideoSource, I think it is better rename VideoProducer to VideoSink or VideoDestination as well.Thought? BTW, I have another idea. MediaStreamVideoTrack and MediaStreamAudioTrack are almost same. The only different is the |Configure()| function. I think we can merge them to one interface MediaStreamTrackSource. And provide two configure functions (ConfigureVideo() and ConfigureAudio()), and add GetType() function for querying source type. It could save some code. And same idea for VideoProducer/AudioProducer. Thought? https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:42: PP_VIDEOFRAME_FORMAT_YV12J = 7 On 2013/12/20 17:42:52, dmichael wrote: > Why is this 7? > > > Are all of these uncompressed, and so a frame has fixed size? (pardon my > ignorance) I think it is uncompressed too. Just the color range is slight different. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:66: void SetTimestamp([in] PP_Resource resource, [in] PP_TimeDelta timestamp); On 2013/12/20 17:42:52, dmichael wrote: > Hmm, you know, any Pepper calls we make require acquiring the proxy lock, which > might be heavily contested. Would it make sense at all to instead make the > timestamp a parameter to the PutFrame function (or whatever it will be called)? What about the GetTimestamp() and other Get functions? Maybe we should use a VideoFrameInfo c structure, and then PutFrame(), GetFrame(), etc will look like: PutFrame(const struct VideoFrameInfo* info, mem_t pixels); GetFrame(struct VideoFrameInfo* info, mem_t* pixels); AllocFrame(VideoFrameInfo* info, mem_t* pixels); RecycleFrame(mem_t pixels); Thought?
https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:12: M33 = 0.1 On 2013/12/20 17:52:26, yzshen1 wrote: > I think we can now use the [channel=dev] tag, because Justin has landed the > support: > https://code.google.com/p/chromium/issues/detail?id=325403 Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:27: PP_Bool IsMediaStreamVideoTrack([in]PP_Resource resource); On 2013/12/20 17:42:52, dmichael wrote: > style/consistency nit: please put a space between [in] and the parameter type > (here and elsewhere) Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:30: * Configure underlayer frame buffers for incoming frames. The provided On 2013/12/20 17:42:52, dmichael wrote: > underlayer->underlying? Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:30: * Configure underlayer frame buffers for incoming frames. The provided On 2013/12/20 17:52:26, yzshen1 wrote: > - Configure*s* > - underlayer -> underlying? Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:31: * |frame_buffer_size| is provided in terms of the number of frames to use for On 2013/12/20 17:52:26, yzshen1 wrote: > Other methods use @param[in] @return tag, please make them consistent. Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:34: * inter-frame processing time variability won't overrun the ring buffer. On 2013/12/20 17:52:26, yzshen1 wrote: > Nit: no need to mention 'ring' buffer. That is an implementation detail. Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:39: int32_t Configure([in]PP_Resource resource, [in]uint32_t frame_buffer_size); On 2013/12/20 17:42:52, dmichael wrote: > Here and in methods other than "IsMediaStreamVideoTrack", we would usually use a > more specific name than |resource| to imply that it must be a > MediaStreamVideoTrack resource. "video_track" is probably sufficient. Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:49: PP_Var GetId([in]PP_Resource resource); On 2013/12/20 17:52:26, yzshen1 wrote: > Except the IsMediaStreamVideoTrack() method, please use > |media_stream_video_track| as parameter name instead of |resource|. (here and > below) Can I use video_track? media_stream_video_track is too long. Is using a full name for resource a strict rule in pepper API style? Done https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:68: * another frame to be buffered. On 2013/12/20 17:52:26, yzshen1 wrote: > It seems nice to also comment that once the input buffer is full, further > GetFrame() calls won't complete until ReuseFrame() is called to free up buffer > slots. To make think easy. I plan to return error, if the caller holds a frame got remove previous |GetFrame()| call. So the call must recycle frames, before using this function. Done https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:78: * Returns PP_ERROR_BADRESOURCE if resource isn't a valid video source. On 2013/12/20 17:42:52, dmichael wrote: > (I don't think you need to list PP_ERROR_BADRESOURCE explicitly) Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:82: * browser error occurs. On 2013/12/20 17:42:52, dmichael wrote: > (Ditto for PP_ERROR_FAILED; these are pretty common and used consistently, so > referencing pp_errors.h is sufficient. Same applies to other places you list > these two.) Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:89: * Release a frame got from |GetFrame()|, so the track can reuse it for On 2013/12/20 17:52:26, yzshen1 wrote: > Release*s* Changed to Recycles https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:90: * new frames.Gets the next video frame from the MediaStream track. On 2013/12/20 17:42:52, dmichael wrote: > I don't think you want the last sentence here... > > Also, it would be good to mention that the given frame is invalid after this > call. Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:90: * new frames.Gets the next video frame from the MediaStream track. On 2013/12/20 17:52:26, yzshen1 wrote: > - One space before "Gets", please. > > - Please talk about "the application shouldn't use |frame| anymore and should > release references it holds to |frame|. Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:104: int32_t ReuseFrame([in] PP_Resource resource, On 2013/12/20 17:42:52, dmichael wrote: > I think we might be able to come up with a better name. "ReuseFrame" seems to > imply the frame is going to get used for something as a result of this call, > when really it's just telling the track that we're done with it. > One possible alternative is "RecycleFrame". Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:108: * Closes the MediaStream video track. On 2013/12/20 17:52:26, yzshen1 wrote: > Please talk about whether it is allowed to do Configure() / GetFrame() / etc. > after Close(). (I think we probably don't need to support re-Configure(), > right?) Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:57: * Get timestamp of the video frame. Given in seconds since the On 2013/12/20 17:52:26, yzshen1 wrote: > Get*s* (Here and similar issues below.) Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:58: * start of the containing video stream. On 2013/12/20 17:52:26, yzshen1 wrote: > You could use @param[in] @return to be consistent. Done. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:60: PP_TimeDelta GetTimestamp([in] PP_Resource resource); On 2013/12/20 17:52:26, yzshen1 wrote: > Please change all |resource| below to |video_frame|. Done.
Mostly nits. Thanks! https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:49: PP_Var GetId([in]PP_Resource resource); > Can I use video_track? media_stream_video_track is too long. Is using a full > name for resource a strict rule in pepper API style? Sounds good. https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:68: * another frame to be buffered. I think you should also comment what would happen if the producer side is too slow and currently there is no contents in the buffer, I think the completion callback won't return until a frame arrives, right? https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_video_fram... ppapi/api/ppb_video_frame.idl:66: void SetTimestamp([in] PP_Resource resource, [in] PP_TimeDelta timestamp); IMO, the current PP_Resource way seems cleaner. I think the performance won't be too bad, we have many PP_Resources that have getter/setters. What do you think? On 2013/12/20 18:49:17, Peng wrote: > On 2013/12/20 17:42:52, dmichael wrote: > > Hmm, you know, any Pepper calls we make require acquiring the proxy lock, > which > > might be heavily contested. Would it make sense at all to instead make the > > timestamp a parameter to the PutFrame function (or whatever it will be > called)? > > What about the GetTimestamp() and other Get functions? Maybe we should use a > VideoFrameInfo c structure, and then PutFrame(), GetFrame(), etc will look like: > > PutFrame(const struct VideoFrameInfo* info, mem_t pixels); > GetFrame(struct VideoFrameInfo* info, mem_t* pixels); > AllocFrame(VideoFrameInfo* info, mem_t* pixels); > RecycleFrame(mem_t pixels); > > Thought? > https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:12: [channel=dev] M33 = 0.1 M34? :) Time flies. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:33: * variability won't overrun the input buffer. If the buffer is overfilled, nit: extra space before 'input'. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:75: * another frame to be buffered. If the caller holds a frame got from previous *the* previous call https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:76: * call of |GetFrame()|, an error will be returned. The caller should recycle what error will be returned in this case? https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:77: * previous frame, before getting next frame. *the* previous frame. *the* next frame. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:96: * the underlaying buffer of this frame. And the frame will become invalidate. invalidate -> invalid https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:102: * @param[in] video_frame A <code>PP_Resource</code> corresponding to a the parameter name is |frame|. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:114: * And then not new frames will be recevied. After calling |Close()|, the not -> no. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_video_fra... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:10: [channel=dev] M33 = 0.1 M34? https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.cc (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.cc:69: } it seems you haven't updated this file. I will review it later. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.h (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:14: /// This file defines the <code>MediaStreamTrackVideo</code> interface for a TrackVideo -> VideoTrack. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:34: /// @param[in] resource A <code>PPB_ MediaStreamVideoTrack</code> resource. extra space after PPB_ https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:37: /// The destructor. This comment seems useless. :) Maybe we could remove it. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:40: /// Configure underlayer frame buffers for incoming frames. The provided Please update the comments to match the C API comments. (here and below) https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:87: int32_t ReuseFrame(const VideoFrame& frame); Recycle? https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:88: }; Close()? https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.cc File ppapi/cpp/video_frame.cc (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:21: : Resource(PASS_REF, resource) { wrong indent. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:28: if (!has_interface<PPB_VideoFrame_0_1>()) { The condition of if() is incorrect. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:36: if (has_interface<PPB_VideoFrame_0_1>()) { nit: no need to have {} https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:42: if (!has_interface<PPB_VideoFrame_0_1>()) nit, optional: I would prefer to always test has_interface() or !has_interface(). Usually we test has_interface() because it is easy to add fallbacks when there are multiple versions of API. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:63: int32_t size = 0; uint32_t https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:68: return (size >= 0) ? size : 0; You don't need this check if |size| is uint32_t https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.h File ppapi/cpp/video_frame.h (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:25: const Size GetSize() const; Is it better to use bool GetSize(Size* size) to be consistent with the C API?
Comment comments. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:36: * settings will be used. Could you make it clearer that this is the number of video frames in the buffer, rather than the actual size in bytes? https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:95: * Recycles a frame got from |GetFrame()|, so the track can reuse s/got/gotten but really 'returned by' might be less awkward in this situation. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:106: * Retruns PP_ERROR_BADARGUMENT if frame isn't a vaild video frame got from see comment above. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:115: * video track should not be used anymore. How about this? * Closes the MediaStream video track, and disconnects it from video source. * After calling Close, no new frames will be received.
https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:12: [channel=dev] M33 = 0.1 On 2013/12/20 23:37:43, yzshen1 wrote: > M34? :) Time flies. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:33: * variability won't overrun the input buffer. If the buffer is overfilled, On 2013/12/20 23:37:43, yzshen1 wrote: > nit: extra space before 'input'. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:36: * settings will be used. On 2013/12/20 23:58:13, bbudge1 wrote: > Could you make it clearer that this is the number of video frames in the buffer, > rather than the actual size in bytes? Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:75: * another frame to be buffered. If the caller holds a frame got from previous On 2013/12/20 23:37:43, yzshen1 wrote: > *the* previous call Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:76: * call of |GetFrame()|, an error will be returned. The caller should recycle On 2013/12/20 23:37:43, yzshen1 wrote: > what error will be returned in this case? Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:77: * previous frame, before getting next frame. On 2013/12/20 23:37:43, yzshen1 wrote: > *the* previous frame. *the* next frame. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:95: * Recycles a frame got from |GetFrame()|, so the track can reuse On 2013/12/20 23:58:13, bbudge1 wrote: > s/got/gotten but really 'returned by' might be less awkward in this situation. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:96: * the underlaying buffer of this frame. And the frame will become invalidate. On 2013/12/20 23:37:43, yzshen1 wrote: > invalidate -> invalid Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:102: * @param[in] video_frame A <code>PP_Resource</code> corresponding to a On 2013/12/20 23:37:43, yzshen1 wrote: > the parameter name is |frame|. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:106: * Retruns PP_ERROR_BADARGUMENT if frame isn't a vaild video frame got from On 2013/12/20 23:58:13, bbudge1 wrote: > see comment above. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:114: * And then not new frames will be recevied. After calling |Close()|, the On 2013/12/20 23:37:43, yzshen1 wrote: > not -> no. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:115: * video track should not be used anymore. On 2013/12/20 23:58:13, bbudge1 wrote: > How about this? > > * Closes the MediaStream video track, and disconnects it from video source. > * After calling Close, no new frames will be received. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_video_fra... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:10: [channel=dev] M33 = 0.1 On 2013/12/20 23:37:43, yzshen1 wrote: > M34? Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.cc (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.cc:69: } On 2013/12/20 23:37:43, yzshen1 wrote: > it seems you haven't updated this file. I will review it later. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.h (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:14: /// This file defines the <code>MediaStreamTrackVideo</code> interface for a On 2013/12/20 23:37:43, yzshen1 wrote: > TrackVideo -> VideoTrack. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:34: /// @param[in] resource A <code>PPB_ MediaStreamVideoTrack</code> resource. On 2013/12/20 23:37:43, yzshen1 wrote: > extra space after PPB_ Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:37: /// The destructor. On 2013/12/20 23:37:43, yzshen1 wrote: > This comment seems useless. :) Maybe we could remove it. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:40: /// Configure underlayer frame buffers for incoming frames. The provided On 2013/12/20 23:37:43, yzshen1 wrote: > Please update the comments to match the C API comments. (here and below) Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:87: int32_t ReuseFrame(const VideoFrame& frame); On 2013/12/20 23:37:43, yzshen1 wrote: > Recycle? Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:88: }; On 2013/12/20 23:37:43, yzshen1 wrote: > Close()? Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.cc File ppapi/cpp/video_frame.cc (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:21: : Resource(PASS_REF, resource) { On 2013/12/20 23:37:43, yzshen1 wrote: > wrong indent. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:28: if (!has_interface<PPB_VideoFrame_0_1>()) { On 2013/12/20 23:37:43, yzshen1 wrote: > The condition of if() is incorrect. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:36: if (has_interface<PPB_VideoFrame_0_1>()) { On 2013/12/20 23:37:43, yzshen1 wrote: > nit: no need to have {} Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:42: if (!has_interface<PPB_VideoFrame_0_1>()) On 2013/12/20 23:37:43, yzshen1 wrote: > nit, optional: I would prefer to always test has_interface() or > !has_interface(). Usually we test has_interface() because it is easy to add > fallbacks when there are multiple versions of API. Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:63: int32_t size = 0; On 2013/12/20 23:37:43, yzshen1 wrote: > uint32_t Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.c... ppapi/cpp/video_frame.cc:68: return (size >= 0) ? size : 0; On 2013/12/20 23:37:43, yzshen1 wrote: > You don't need this check if |size| is uint32_t Done. https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.h File ppapi/cpp/video_frame.h (right): https://codereview.chromium.org/107083004/diff/150001/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:25: const Size GetSize() const; On 2013/12/20 23:37:43, yzshen1 wrote: > Is it better to use bool GetSize(Size* size) to be consistent with the C API? Done.
A few more nits. Thanks! https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:68: * another frame to be buffered. On 2013/12/20 23:37:43, yzshen1 wrote: > I think you should also comment what would happen if the producer side is too > slow and currently there is no contents in the buffer, I think the completion > callback won't return until a frame arrives, right? In case you didn't notice this comment... https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:120: int32_t Close([in] PP_Resource video_track); I think all Close() methods of other APIs return void. Is there a reason why we need int32_t here? Otherwise, I wish they could be consistent. https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:95: * @param[out] size A <code>PP_Size</code> nit: Add trailing '.', please. https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:97: * @return A <code>PP_Bool</code> with <code>PP_TRUE</code> if gets size nit: maybe we could consider "A <code>PP_Bool</code> with <code>PP_TRUE</code> on success or <code>PP_FALSE</code> on failure." https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:98: * successfully, otherwise <code>PP_FALSE</code> is returned. extra space before 'is'. https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.h (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:34: /// @para[in] resource A <code>PPB_MediaStreamVideoTrack</code> resource. para -> param https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/video_frame.h File ppapi/cpp/video_frame.h (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:50: /// @param[out] size A <code>PP_Size</code> Please update the comment, it is not PP_Size here. https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:52: /// @return True if gets size successfully, otherwise false is returned. extra space before 'is'.
https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:120: int32_t Close([in] PP_Resource video_track); On 2013/12/26 17:52:22, yzshen1 wrote: > I think all Close() methods of other APIs return void. Is there a reason why we > need int32_t here? Otherwise, I wish they could be consistent. Done. https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:95: * @param[out] size A <code>PP_Size</code> On 2013/12/26 17:52:22, yzshen1 wrote: > nit: Add trailing '.', please. Done. https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:97: * @return A <code>PP_Bool</code> with <code>PP_TRUE</code> if gets size On 2013/12/26 17:52:22, yzshen1 wrote: > nit: maybe we could consider > "A <code>PP_Bool</code> with <code>PP_TRUE</code> on success or > <code>PP_FALSE</code> on failure." Done. https://codereview.chromium.org/107083004/diff/330002/ppapi/api/ppb_video_fra... ppapi/api/ppb_video_frame.idl:98: * successfully, otherwise <code>PP_FALSE</code> is returned. On 2013/12/26 17:52:22, yzshen1 wrote: > extra space before 'is'. Done. https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.h (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:34: /// @para[in] resource A <code>PPB_MediaStreamVideoTrack</code> resource. On 2013/12/26 17:52:22, yzshen1 wrote: > para -> param Done. https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/video_frame.h File ppapi/cpp/video_frame.h (right): https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:50: /// @param[out] size A <code>PP_Size</code> On 2013/12/26 17:52:22, yzshen1 wrote: > Please update the comment, it is not PP_Size here. Done. https://codereview.chromium.org/107083004/diff/330002/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:52: /// @return True if gets size successfully, otherwise false is returned. On 2013/12/26 17:52:22, yzshen1 wrote: > extra space before 'is'. Done.
LGTM with a few nits. BTW, I think I saw you commented somewhere that you are considering using a single API to deal with both audio and video. I don't have strong opinion about that, only one thing to consider: are you planing to let GetFrame()/etc. return a generic pp::Resource object and let user cast it to VideoFrame/AudioFrame? It seems to be a little bit extra work and "type unsafe". https://codereview.chromium.org/107083004/diff/450002/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/450002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:32: * |max_buffered_frames| should be chosen such that inter-frame nit: it seems you use both || and <code></code>, it is better to be consistent and change || to <code></code>. What do you think? https://codereview.chromium.org/107083004/diff/450002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:78: * the next frame. (I have commented about this twice but you might not notice. You probably haven't read the comments on older patchsets.) I think you should also comment what would happen if the producer side is too slow and currently there is no contents in the buffer, I think the completion callback won't return until a frame arrives, right? https://codereview.chromium.org/107083004/diff/450002/ppapi/cpp/video_frame.h File ppapi/cpp/video_frame.h (right): https://codereview.chromium.org/107083004/diff/450002/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:57: /// @param[out] size A <code>Size</code> trailing '.', please.
On 2013/12/26 18:52:37, yzshen1 wrote: > LGTM with a few nits. > > BTW, I think I saw you commented somewhere that you are considering using a > single API to deal with both audio and video. I don't have strong opinion about > that, only one thing to consider: are you planing to let GetFrame()/etc. return > a generic pp::Resource object and let user cast it to VideoFrame/AudioFrame? It > seems to be a little bit extra work and "type unsafe". I think the solution could be we have one c interface for audio and video,but in c++ layer we could have two different tracks (pp:MediaStreamAudioTrack/VideoTrack). But I saw all pepper API c++ is just a simple wrapper, not sure if adding more logic in c++ layer is good.
https://codereview.chromium.org/107083004/diff/450002/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/450002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:32: * |max_buffered_frames| should be chosen such that inter-frame On 2013/12/26 18:52:38, yzshen1 wrote: > nit: it seems you use both || and <code></code>, it is better to be consistent > and change || to <code></code>. What do you think? Sure. Done https://codereview.chromium.org/107083004/diff/450002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:78: * the next frame. On 2013/12/26 18:52:38, yzshen1 wrote: > (I have commented about this twice but you might not notice. You probably > haven't read the comments on older patchsets.) > > I think you should also comment what would happen if the producer side is too > slow and currently there is no contents in the buffer, I think the completion > callback won't return until a frame arrives, right? Sorry. I just used hotkey to navigate comments in very files. So I didn't see the comment in only patchset. Added comment about it. Done Done https://codereview.chromium.org/107083004/diff/450002/ppapi/cpp/video_frame.h File ppapi/cpp/video_frame.h (right): https://codereview.chromium.org/107083004/diff/450002/ppapi/cpp/video_frame.h... ppapi/cpp/video_frame.h:57: /// @param[out] size A <code>Size</code> On 2013/12/26 18:52:38, yzshen1 wrote: > trailing '.', please. Done.
> I think the solution could be we have one c interface for audio and video,but in > c++ > layer we could have two different tracks (pp:MediaStreamAudioTrack/VideoTrack). > But I saw all pepper API c++ is just a simple wrapper, not sure if adding more > logic in c++ layer is good. I think the guideline is to have a simple and straightforward mapping between C and C++ APIs. Maybe we can consider sharing the implementation while keeping the C/C++ APIs separate? https://codereview.chromium.org/107083004/diff/790002/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/790002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:77: * <code>PP_OK_COMPLETIONPENDING</code> will be returned immediatelly. And the nit: immediate*l*y. https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.cc (right): https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.cc:31: MediaStreamVideoTrack::MediaStreamVideoTrack(const Resource& resource) Do we need this method now that we already have the PassRef constructor? Besides, please note that you don't check IsMediaStreamVideoTrack() for the PassRef constructor. I think it is okay not to check, the outcome is all calls to the underlying C API will fail afterwards. https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.cc:96: IsMediaStreamVideoTrack(resource.pp_resource()); wrong indent. https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.h (right): https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:78: /// <code>PP_OK_COMPLETIONPENDIN</code> will be returned immediatelly. And the PP_OK_COMPLETIONPENDIN*G*, immediate*l*y https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:117: static bool IsMediaStreamVideoTrack(const Resource& resource); It seems we don't have this method for other cpp APIs.
On Fri, Dec 27, 2013 at 10:40 AM, <yzshen@chromium.org> wrote: >> I think the solution could be we have one c interface for audio and >> video,but > > in >> >> c++ >> layer we could have two different tracks > > (pp:MediaStreamAudioTrack/VideoTrack). >> >> But I saw all pepper API c++ is just a simple wrapper, not sure if adding > > more >> >> logic in c++ layer is good. > > > I think the guideline is to have a simple and straightforward mapping > between C > and C++ APIs. Yes, please! > > Maybe we can consider sharing the implementation while keeping the C/C++ > APIs > separate? > > > https://codereview.chromium.org/107083004/diff/790002/ppapi/api/ppb_media_str... > File ppapi/api/ppb_media_stream_video_track.idl (right): > > https://codereview.chromium.org/107083004/diff/790002/ppapi/api/ppb_media_str... > ppapi/api/ppb_media_stream_video_track.idl:77: * > <code>PP_OK_COMPLETIONPENDING</code> will be returned immediatelly. And > the > nit: immediate*l*y. > > https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... > File ppapi/cpp/media_stream_video_track.cc (right): > > https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... > ppapi/cpp/media_stream_video_track.cc:31: > MediaStreamVideoTrack::MediaStreamVideoTrack(const Resource& resource) > Do we need this method now that we already have the PassRef constructor? > > Besides, please note that you don't check IsMediaStreamVideoTrack() for > the PassRef constructor. I think it is okay not to check, the outcome is > all calls to the underlying C API will fail afterwards. > > https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... > ppapi/cpp/media_stream_video_track.cc:96: > IsMediaStreamVideoTrack(resource.pp_resource()); > wrong indent. > > https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... > File ppapi/cpp/media_stream_video_track.h (right): > > https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... > ppapi/cpp/media_stream_video_track.h:78: /// > <code>PP_OK_COMPLETIONPENDIN</code> will be returned immediatelly. And > the > PP_OK_COMPLETIONPENDIN*G*, immediate*l*y > > https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... > ppapi/cpp/media_stream_video_track.h:117: static bool > IsMediaStreamVideoTrack(const Resource& resource); > It seems we don't have this method for other cpp APIs. > > https://codereview.chromium.org/107083004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/107083004/diff/790002/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/790002/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:77: * <code>PP_OK_COMPLETIONPENDING</code> will be returned immediatelly. And the On 2013/12/27 17:40:08, yzshen1 wrote: > nit: immediate*l*y. Done. https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.cc (right): https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.cc:31: MediaStreamVideoTrack::MediaStreamVideoTrack(const Resource& resource) On 2013/12/27 17:40:08, yzshen1 wrote: > Do we need this method now that we already have the PassRef constructor? > > Besides, please note that you don't check IsMediaStreamVideoTrack() for the > PassRef constructor. I think it is okay not to check, the outcome is all calls > to the underlying C API will fail afterwards. > > This constructor is used for passing js obj to NaCl. In NaCl, the code looks like: + pp::VarDictionary var_dictionary_message(var_message); + pp::Var var_track = var_dictionary_message.Get("track"); + if (!var_track.is_resource()) + return; + + pp::Resource resource_track = pp::VarResource_Dev(var_track).AsResource(); + + video_track_ = pp::MediaStreamVideoTrack(resource_track); As discussed it offline, we need keep this constructor, but change PP_NOREACHED() to PP_DCHECK(). https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.cc:96: IsMediaStreamVideoTrack(resource.pp_resource()); On 2013/12/27 17:40:08, yzshen1 wrote: > wrong indent. Done. https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... File ppapi/cpp/media_stream_video_track.h (right): https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:78: /// <code>PP_OK_COMPLETIONPENDIN</code> will be returned immediatelly. And the On 2013/12/27 17:40:08, yzshen1 wrote: > PP_OK_COMPLETIONPENDIN*G*, immediate*l*y Done. https://codereview.chromium.org/107083004/diff/790002/ppapi/cpp/media_stream_... ppapi/cpp/media_stream_video_track.h:117: static bool IsMediaStreamVideoTrack(const Resource& resource); On 2013/12/27 17:40:08, yzshen1 wrote: > It seems we don't have this method for other cpp APIs. It is useful for passing js object to NaCl.
https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/80001/ppapi/api/ppb_media_stre... ppapi/api/ppb_media_stream_video_track.idl:8: * receiving video frames from a MediaStream video track in the browser. On 2013/12/20 18:49:17, Peng wrote: > On 2013/12/20 17:42:52, dmichael wrote: > > What's the plan for pushing frames to a track? Will that eventually go in this > > API, or will that be a separate one? I ask mainly because the name you have > here > > seems to imply it will do both (or it will be hard to name the one for pushing > > frames). > > For pushing frames, we will have a separate one. > How about using MediaStreamVideoSource / MediaStreamVideoTrackSource? I feel > MSVTS is too long. :( And if we use *VideoSource, I think it is better rename > VideoProducer to VideoSink or VideoDestination as well.Thought? > > BTW, I have another idea. MediaStreamVideoTrack and MediaStreamAudioTrack are > almost same. The only different is the |Configure()| function. I think we can > merge them to one interface MediaStreamTrackSource. And provide two configure > functions (ConfigureVideo() and ConfigureAudio()), and add GetType() function > for querying source type. It could save some code. And same idea for > VideoProducer/AudioProducer. Thought? > As I discussed with Yuzhu. I am going to land this CL first, and then we could discuss the class name and rename it in a separate CL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/107083004/1090001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2013/12/27 21:39:49, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... I think it is better to wait for LGTMs from all reviewers. What do you think?
Hi noelallen, please review native_client_sdk/*. Thanks.
Just comment nits. LGTM I think you'll need a LGTM from a NaClSDK OWNER to use CQ. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:30: * Configures underlaying frame buffers for incoming frames. s/underlaying/underlying https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:76: * If there is no frames in the input buffer, s/is/are https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:77: * <code>PP_OK_COMPLETIONPENDING</code> will be returned immediately. And the s/immediately. And/immediately and https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:79: * error happens. s/recieved/received s/some error happens/an error occurs https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:81: * <code>GetFrame()</code>, <code>PP_ERROR_INGROGRESS</code> will be returned. s/PP_ERROR_INGROGRESS/PP_ERROR_INPROGRESS https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:82: * The caller should recycle the previous frame, before getting the next s/frame,/frame https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:102: * reuse the underlaying buffer of this frame. And the frame will become s/underlaying/underlying s/And the/The https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:104: * <code>frame</code>, and not use it anymore. s/, and/and https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:117: * Closes the MediaStream video track, and disconnects it from video source. s/, and/ and https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:57: * Gets timestamp of the video frame. s/timestamp/the timestamp https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:62: * @return A <code>PP_TimeDelta</code> containing timestamp of the video s/timestamp/the timestamp https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:80: * Gets format of the video frame. s/format/the format https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:91: * Gets size of the video frame. s/size/the size https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:103: * Gets data buffer for video frame pixels. s/data buffer/the data buffer https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:108: * @return A pointer to the beginning of data buffer. s/data/the data https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:113: * Gets size of data buffer. s/size/the size
On 2013/12/27 22:04:17, bbudge1 wrote: > Just comment nits. LGTM > > I think you'll need a LGTM from a NaClSDK OWNER to use CQ. > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > File ppapi/api/ppb_media_stream_video_track.idl (right): > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:30: * Configures underlaying frame > buffers for incoming frames. > s/underlaying/underlying > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:76: * If there is no frames in the > input buffer, > s/is/are > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:77: * > <code>PP_OK_COMPLETIONPENDING</code> will be returned immediately. And the > s/immediately. And/immediately and > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:79: * error happens. > s/recieved/received > > s/some error happens/an error occurs > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:81: * <code>GetFrame()</code>, > <code>PP_ERROR_INGROGRESS</code> will be returned. > s/PP_ERROR_INGROGRESS/PP_ERROR_INPROGRESS > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:82: * The caller should recycle the > previous frame, before getting the next > s/frame,/frame > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:102: * reuse the underlaying buffer > of this frame. And the frame will become > s/underlaying/underlying > s/And the/The > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:104: * <code>frame</code>, and not > use it anymore. > s/, and/and > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... > ppapi/api/ppb_media_stream_video_track.idl:117: * Closes the MediaStream video > track, and disconnects it from video source. > s/, and/ and > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > File ppapi/api/ppb_video_frame.idl (right): > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > ppapi/api/ppb_video_frame.idl:57: * Gets timestamp of the video frame. > s/timestamp/the timestamp > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > ppapi/api/ppb_video_frame.idl:62: * @return A <code>PP_TimeDelta</code> > containing timestamp of the video > s/timestamp/the timestamp > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > ppapi/api/ppb_video_frame.idl:80: * Gets format of the video frame. > s/format/the format > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > ppapi/api/ppb_video_frame.idl:91: * Gets size of the video frame. > s/size/the size > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > ppapi/api/ppb_video_frame.idl:103: * Gets data buffer for video frame pixels. > s/data buffer/the data buffer > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > ppapi/api/ppb_video_frame.idl:108: * @return A pointer to the beginning of data > buffer. > s/data/the data > > https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... > ppapi/api/ppb_video_frame.idl:113: * Gets size of data buffer. > s/size/the size Also, please propagate IDL changes to C++ wrapper comments.
https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:30: * Configures underlaying frame buffers for incoming frames. On 2013/12/27 22:04:18, bbudge1 wrote: > s/underlaying/underlying Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:76: * If there is no frames in the input buffer, On 2013/12/27 22:04:18, bbudge1 wrote: > s/is/are Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:77: * <code>PP_OK_COMPLETIONPENDING</code> will be returned immediately. And the On 2013/12/27 22:04:18, bbudge1 wrote: > s/immediately. And/immediately and Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:79: * error happens. On 2013/12/27 22:04:18, bbudge1 wrote: > s/recieved/received > > s/some error happens/an error occurs Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:81: * <code>GetFrame()</code>, <code>PP_ERROR_INGROGRESS</code> will be returned. On 2013/12/27 22:04:18, bbudge1 wrote: > s/PP_ERROR_INGROGRESS/PP_ERROR_INPROGRESS Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:82: * The caller should recycle the previous frame, before getting the next On 2013/12/27 22:04:18, bbudge1 wrote: > s/frame,/frame Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:102: * reuse the underlaying buffer of this frame. And the frame will become On 2013/12/27 22:04:18, bbudge1 wrote: > s/underlaying/underlying > s/And the/The Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:104: * <code>frame</code>, and not use it anymore. On 2013/12/27 22:04:18, bbudge1 wrote: > s/, and/and Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_media_st... ppapi/api/ppb_media_stream_video_track.idl:117: * Closes the MediaStream video track, and disconnects it from video source. On 2013/12/27 22:04:18, bbudge1 wrote: > s/, and/ and Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... File ppapi/api/ppb_video_frame.idl (right): https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:57: * Gets timestamp of the video frame. On 2013/12/27 22:04:18, bbudge1 wrote: > s/timestamp/the timestamp Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:62: * @return A <code>PP_TimeDelta</code> containing timestamp of the video On 2013/12/27 22:04:18, bbudge1 wrote: > s/timestamp/the timestamp Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:80: * Gets format of the video frame. On 2013/12/27 22:04:18, bbudge1 wrote: > s/format/the format Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:91: * Gets size of the video frame. On 2013/12/27 22:04:18, bbudge1 wrote: > s/size/the size Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:103: * Gets data buffer for video frame pixels. On 2013/12/27 22:04:18, bbudge1 wrote: > s/data buffer/the data buffer Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:108: * @return A pointer to the beginning of data buffer. On 2013/12/27 22:04:18, bbudge1 wrote: > s/data/the data Done. https://codereview.chromium.org/107083004/diff/1090001/ppapi/api/ppb_video_fr... ppapi/api/ppb_video_frame.idl:113: * Gets size of data buffer. On 2013/12/27 22:04:18, bbudge1 wrote: > s/size/the size Done.
try sbc@ & binji@. Hi Sam or Ben, Could you please review changes in native_client_sdk? Thanks.
native_client_sdk lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/107083004/1440001
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...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/107083004/1760001
Retried try job too often on win_rel for step(s) browser_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/107083004/1760001
Message was sent while issue was closed.
Change committed as 243099 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
