Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150403006:
[PPAPI][MediaStream] Support configure for video input. (Closed)
|
Created:
6 years, 10 months ago by Peng Modified:
6 years, 10 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, jam, yzshen+watch_chromium.org, joi+watch-content_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, raymes+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org, kmixter1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[PPAPI][MediaStream] Support configure for video input.
Support configuring frame format and size for video input.
BUG=330851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253307
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Update #
Total comments: 20
Patch Set 4 : Update #
Total comments: 52
Patch Set 5 : Fix review issues #Patch Set 6 : Update #
Total comments: 10
Patch Set 7 : Fix review issues #
Total comments: 8
Patch Set 8 : Fix review issues #
Total comments: 6
Patch Set 9 : Fix review issues #
Total comments: 10
Patch Set 10 : Fix review issues #Patch Set 11 : #
Total comments: 13
Patch Set 12 : Fix review issues #
Total comments: 2
Patch Set 13 : Fix review issues #
Total comments: 2
Patch Set 14 : Fix review issues #
Total comments: 1
Patch Set 15 : Update #Patch Set 16 : Rebase #Patch Set 17 : Rebase #
Total comments: 2
Patch Set 18 : Use libyuv:I420ToARGB() #Patch Set 19 : Fix build errors #Patch Set 20 : Fix build errors #Messages
Total messages: 77 (0 generated)
Hi Yuzhu & Ronghua, I implement a CL for configuring the input video frame format & size. PTAL. Thanks.
first pass https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:20: using ppapi::proxy::SerializedHandle; order https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:51: gfx::Size ComputeSize(const gfx::Size& frame, ComputeSize and ComputeFormat are not so obvious. Please add comment. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:65: const gfx::Size& dst_size) { nit, prefer to have read only args and then read/write args. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:81: media::ScaleYUVToRGB32(src->data(VideoFrame::kYPlane), I think call ScaleYUVToRGB32 with the same size is equal to ConvertYUVToRGB32. But please double check the code to confirm. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:98: static const size_t kPlanes[][3] = { kPlanesOrder? https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:114: libyuv::ScalePlane(src->data(kPlanes[plane_order][0]), dito, I think ScalePlane will just do copy if the size is the same. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:114: libyuv::ScalePlane(src->data(kPlanes[plane_order][0]), And I think you should be able to just use I420Scale instead of ScalePlane. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:206: buffers_initialized_ = true; set buffers_initialized_ inside InitBuffers https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:299: // new settings. Otherwise, we will initialize buffer when we receive why do we need to "initialize buffer when receive first frame" if never be initialized before? https://codereview.chromium.org/150403006/diff/50001/ppapi/shared_impl/media_... File ppapi/shared_impl/media_stream_video_track_shared.cc (right): https://codereview.chromium.org/150403006/diff/50001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_video_track_shared.cc:24: } else if (attributes.buffers) { can the value 0 means not set instead of using an additional mask?
https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:20: using ppapi::proxy::SerializedHandle; On 2014/02/12 19:54:15, Ronghua Wu wrote: > order Sorry. Which one is not in order? https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:51: gfx::Size ComputeSize(const gfx::Size& frame, On 2014/02/12 19:54:15, Ronghua Wu wrote: > ComputeSize and ComputeFormat are not so obvious. Please add comment. Done. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:65: const gfx::Size& dst_size) { On 2014/02/12 19:54:15, Ronghua Wu wrote: > nit, prefer to have read only args and then read/write args. Done. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:81: media::ScaleYUVToRGB32(src->data(VideoFrame::kYPlane), On 2014/02/12 19:54:15, Ronghua Wu wrote: > I think call ScaleYUVToRGB32 with the same size is equal to ConvertYUVToRGB32. > But please double check the code to confirm. I am not sure. Probably they are not same. I checked code of them. The media::ScaleYUVToRGB32 doesn't call ConvertYUVToRGB32 when sizes are same. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:98: static const size_t kPlanes[][3] = { On 2014/02/12 19:54:15, Ronghua Wu wrote: > kPlanesOrder? Done. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:114: libyuv::ScalePlane(src->data(kPlanes[plane_order][0]), On 2014/02/12 19:54:15, Ronghua Wu wrote: > dito, I think ScalePlane will just do copy if the size is the same. Done. https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:114: libyuv::ScalePlane(src->data(kPlanes[plane_order][0]), On 2014/02/12 19:54:15, Ronghua Wu wrote: > And I think you should be able to just use I420Scale instead of ScalePlane. Because we support YV12 & I420 two formats. Use I420Scale is not convenient. :( https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:206: buffers_initialized_ = true; On 2014/02/12 19:54:15, Ronghua Wu wrote: > set buffers_initialized_ inside InitBuffers because we will called InitBuffers() multiple times. So it is little weird to set it in InitBuffers(). I removed this variable. Done https://codereview.chromium.org/150403006/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_video_track_host.cc:299: // new settings. Otherwise, we will initialize buffer when we receive On 2014/02/12 19:54:15, Ronghua Wu wrote: > why do we need to "initialize buffer when receive first frame" if never be > initialized before? Because plugin may only provide part for attributes (maybe only format without frame size) which are not enough to init buffer. So we have to wait the first frame. https://codereview.chromium.org/150403006/diff/50001/ppapi/shared_impl/media_... File ppapi/shared_impl/media_stream_video_track_shared.cc (right): https://codereview.chromium.org/150403006/diff/50001/ppapi/shared_impl/media_... ppapi/shared_impl/media_stream_video_track_shared.cc:24: } else if (attributes.buffers) { On 2014/02/12 19:54:15, Ronghua Wu wrote: > can the value 0 means not set instead of using an additional mask? My idea is: If the mask is 1 and value is 0. it means to use the default value. if the mask is 0, it means to use old value (set by previous Configure() call) instead the default value. WDYT?
+jschuh@chromium.org +dcheng@chromium.org for IPC changes. Thanks.
https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:20: using ppapi::proxy::SerializedHandle; You are not using it. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:24: const int32_t kNumberOfBuffers = 4; Is it k*Default*....? https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:211: DCHECK(frame->coded_size() == frame_size_); (just to double check) So OnVideoFrame never returns frames of different size or format? I remember you mentioned before that it is possible to change format/size in the middle? https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:225: buffer->data_size = frame_data_size_; I think the naming is confusing: |frame_data_size_| doesn't correspond to |frame_size_/format_|. Instead, it corresponds to either that or |plugin_frame_size_/format_|. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:249: int32_t PepperMediaStreamVideoTrackHost::OnHostMsgConfigure( The API definition: Do we allow Configure() to be called multiple times? Do we allow it to be called after GetFrame() calls? I am afraid of corruptions/races around reconfiguring. For example, the plugin GetFrame(), holds it, re-configure, RecycleFrame(). An index corresponds to the old buffers is reported. There might be other issues. Please think hard about this part. It is extremely easy to get into trouble. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:252: CHECK(MediaStreamVideoTrackShared::VerifyAttributes(attributes)); Should we reply the incoming request with failure instead of crashing ourselves? https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:271: int32_t buffers = attributes.buffers ? So it is always > 0, right? https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:291: // If the first frame has been received, we will re init buffers with re init -> re-initialize https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:295: if (changed && !frame_size_.IsEmpty()) { nit: no need to have {} https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:57: // number of buffers Capital initial and trailing period please. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:47: if (has_ended()) { nit: no need to have {} and line 57. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:204: DCHECK(TrackedCallback::IsPending(configure_callback_)); Please explicitly check if (!IsPending()) return. When the last plugin side ref is deleted, all the callbacks related to that resource are aborted. (See ResourceTracker::LastPluginRefWasDeleted) But it is possible that the Resource object is alive for a while. In that case, you might find out the callback is not pending anymore. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... File ppapi/proxy/ppapi_param_traits.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... ppapi/proxy/ppapi_param_traits.cc:665: r->format = static_cast<PP_VideoFrame_Format>(value); This is not safe, you might get an invalid value if the message is from a compromised plugin process. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... File ppapi/proxy/ppapi_param_traits.h (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... ppapi/proxy/ppapi_param_traits.h:182: ppapi::MediaStreamVideoTrackShared::Attributes> { Is it possible to use IPC_STRUCT_TRAITS_BEGIN/END and IPC_ENUM_TRAITS_MAX_VALUE instead of writing these manually? https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_video_track_shared.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:11: const int32_t kMaxWidth = 4069; why 4069 but not 4096? :) https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:22: if (attributes.buffers < 0) I think it should be <= 0 here, right? https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:24: } else if (attributes.buffers) { Please use explicit numeric check (here and below). https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:29: if (attributes.format < PP_VIDEOFRAME_FORMAT_UNKNOWN || maybe <=? https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:38: if (attributes.width < 0 || attributes.width > kMaxWidth) Maybe <= 0? https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:47: if (attributes.height < 0 || attributes.height > kMaxHeight) Maybe <= 0?
I need a bit more context, so just two comments for now. I'll wait for more cleanup before further security review. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:63: for (;attrib_list[i] != PP_MEDIASTREAMVIDEOTRACK_ATTRIB_NONE; i += 2) { Which process does this come from, and how do you guarantee the terminator is present? https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... File ppapi/proxy/ppapi_param_traits.h (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... ppapi/proxy/ppapi_param_traits.h:182: ppapi::MediaStreamVideoTrackShared::Attributes> { Yes, there doesn't seem to be any reason to implement it this way, and doing so is more error prone and makes the code harder to read.
CL is updated. PTAL. Thanks. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:20: using ppapi::proxy::SerializedHandle; On 2014/02/13 19:23:26, yzshen1 wrote: > You are not using it. Done. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:24: const int32_t kNumberOfBuffers = 4; On 2014/02/13 19:23:26, yzshen1 wrote: > Is it k*Default*....? Done. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:211: DCHECK(frame->coded_size() == frame_size_); On 2014/02/13 19:23:26, yzshen1 wrote: > (just to double check) So OnVideoFrame never returns frames of different size or > format? I remember you mentioned before that it is possible to change > format/size in the middle? Currently, it will be always same. But w3c has an API to change the frame size. But it is not supported. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:225: buffer->data_size = frame_data_size_; On 2014/02/13 19:23:26, yzshen1 wrote: > I think the naming is confusing: |frame_data_size_| doesn't correspond to > |frame_size_/format_|. Instead, it corresponds to either that or > |plugin_frame_size_/format_|. I think plugin_frame_data_size_ is not a good name as well. Currently plugin_ prefix means those value are specified by plugin, but it could be different to the real value. So I renamed frame_format_ and frame_size_ to source_frame_format_, and source_frame_size_. WDYT? Done https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:249: int32_t PepperMediaStreamVideoTrackHost::OnHostMsgConfigure( On 2014/02/13 19:23:26, yzshen1 wrote: > The API definition: Do we allow Configure() to be called multiple times? Do we > allow it to be called after GetFrame() calls? I think we have to support calling it multiple times. Because the first frame could be received before calling configure(). In this case, the underlying buffers have been initialized. If we support it, we will support re-configure as well. > > I am afraid of corruptions/races around reconfiguring. For example, the plugin > GetFrame(), holds it, re-configure, RecycleFrame(). An index corresponds to the > old buffers is reported. Those cases will be checked in MediaStreamVideoTreackResource. If Configure() is called with some frames hold by plugin, the configure() will return error directly. > > There might be other issues. > > Please think hard about this part. It is extremely easy to get into trouble. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:252: CHECK(MediaStreamVideoTrackShared::VerifyAttributes(attributes)); On 2014/02/13 19:23:26, yzshen1 wrote: > Should we reply the incoming request with failure instead of crashing ourselves? Because MediaStreamVideoTreackResource will use this function to check attrib_list as well. So at here, this function should always return true, otherwise the chrome may be hacked. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:271: int32_t buffers = attributes.buffers ? On 2014/02/13 19:23:26, yzshen1 wrote: > So it is always > 0, right? Yes. MediaStreamVideoTrackShared::VerifyAttributes(attributes) will make sure all attributes are valid. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:291: // If the first frame has been received, we will re init buffers with On 2014/02/13 19:23:26, yzshen1 wrote: > re init -> re-initialize Done. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:295: if (changed && !frame_size_.IsEmpty()) { On 2014/02/13 19:23:26, yzshen1 wrote: > nit: no need to have {} Done. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:57: // number of buffers On 2014/02/13 19:23:26, yzshen1 wrote: > Capital initial and trailing period please. Done. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:47: if (has_ended()) { On 2014/02/13 19:23:26, yzshen1 wrote: > nit: no need to have {} and line 57. Done. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:63: for (;attrib_list[i] != PP_MEDIASTREAMVIDEOTRACK_ATTRIB_NONE; i += 2) { On 2014/02/13 20:38:46, Justin Schuh wrote: > Which process does this come from, and how do you guarantee the terminator is > present? It is from plugin. And this function is executed in plugin process. Is it OK? https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:63: for (;attrib_list[i] != PP_MEDIASTREAMVIDEOTRACK_ATTRIB_NONE; i += 2) { On 2014/02/13 20:38:46, Justin Schuh wrote: > Which process does this come from, and how do you guarantee the terminator is > present? The argument is from plugin. And this function is executed in sandboxed plugin process. Is It OK? https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:204: DCHECK(TrackedCallback::IsPending(configure_callback_)); On 2014/02/13 19:23:26, yzshen1 wrote: > Please explicitly check if (!IsPending()) return. > > When the last plugin side ref is deleted, all the callbacks related to that > resource are aborted. (See ResourceTracker::LastPluginRefWasDeleted) But it is > possible that the Resource object is alive for a while. In that case, you might > find out the callback is not pending anymore. Done. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... File ppapi/proxy/ppapi_param_traits.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... ppapi/proxy/ppapi_param_traits.cc:665: r->format = static_cast<PP_VideoFrame_Format>(value); On 2014/02/13 19:23:26, yzshen1 wrote: > This is not safe, you might get an invalid value if the message is from a > compromised plugin process. Done. https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... File ppapi/proxy/ppapi_param_traits.h (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... ppapi/proxy/ppapi_param_traits.h:182: ppapi::MediaStreamVideoTrackShared::Attributes> { On 2014/02/13 19:23:26, yzshen1 wrote: > Is it possible to use IPC_STRUCT_TRAITS_BEGIN/END and IPC_ENUM_TRAITS_MAX_VALUE > instead of writing these manually? Done https://codereview.chromium.org/150403006/diff/120001/ppapi/proxy/ppapi_param... ppapi/proxy/ppapi_param_traits.h:182: ppapi::MediaStreamVideoTrackShared::Attributes> { On 2014/02/13 19:23:26, yzshen1 wrote: > Is it possible to use IPC_STRUCT_TRAITS_BEGIN/END and IPC_ENUM_TRAITS_MAX_VALUE > instead of writing these manually? Done. https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_video_track_shared.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:11: const int32_t kMaxWidth = 4069; On 2014/02/13 19:23:26, yzshen1 wrote: > why 4069 but not 4096? :) oops! Done https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:22: if (attributes.buffers < 0) On 2014/02/13 19:23:26, yzshen1 wrote: > I think it should be <= 0 here, right? buffers == 0 means using default value. https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:24: } else if (attributes.buffers) { On 2014/02/13 19:23:26, yzshen1 wrote: > Please use explicit numeric check (here and below). Sorry. I don't understand this comment. Please rephrase it. https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:29: if (attributes.format < PP_VIDEOFRAME_FORMAT_UNKNOWN || On 2014/02/13 19:23:26, yzshen1 wrote: > maybe <=? Same, if format == PP_VIDEOFRAME_FORMAT_UNKNOWN, chrome will use the default format. https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:38: if (attributes.width < 0 || attributes.width > kMaxWidth) On 2014/02/13 19:23:26, yzshen1 wrote: > Maybe <= 0? same. if width == 0, chrome will use the source video's width. https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:47: if (attributes.height < 0 || attributes.height > kMaxHeight) On 2014/02/13 19:23:26, yzshen1 wrote: > Maybe <= 0? same to width.
https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:211: DCHECK(frame->coded_size() == frame_size_); If the backend changes to support that in the future, this code may break silently. Maybe we should fail more explicitly, for example, discard the frame, log an error and return? On 2014/02/13 21:28:34, Peng wrote: > On 2014/02/13 19:23:26, yzshen1 wrote: > > (just to double check) So OnVideoFrame never returns frames of different size > or > > format? I remember you mentioned before that it is possible to change > > format/size in the middle? > > Currently, it will be always same. But w3c has an API to change the frame size. > But it is not supported. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:225: buffer->data_size = frame_data_size_; On 2014/02/13 21:28:34, Peng wrote: > On 2014/02/13 19:23:26, yzshen1 wrote: > > I think the naming is confusing: |frame_data_size_| doesn't correspond to > > |frame_size_/format_|. Instead, it corresponds to either that or > > |plugin_frame_size_/format_|. > > I think plugin_frame_data_size_ is not a good name as well. Currently plugin_ > prefix means those value are specified by plugin, but it could be different to > the real value. > > So I renamed frame_format_ and frame_size_ to source_frame_format_, and > source_frame_size_. WDYT? > > Done Okay. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:249: int32_t PepperMediaStreamVideoTrackHost::OnHostMsgConfigure( On 2014/02/13 21:28:34, Peng wrote: > On 2014/02/13 19:23:26, yzshen1 wrote: > > The API definition: Do we allow Configure() to be called multiple times? Do we > > allow it to be called after GetFrame() calls? > > I think we have to support calling it multiple times. Because the first frame > could be received before calling configure(). In this case, the underlying > buffers have been initialized. If we support it, we will support re-configure as > well. > > > > > > I am afraid of corruptions/races around reconfiguring. For example, the plugin > > GetFrame(), holds it, re-configure, RecycleFrame(). An index corresponds to > the > > old buffers is reported. > > Those cases will be checked in MediaStreamVideoTreackResource. If Configure() is > called with some frames hold by plugin, the configure() will return error > directly. I feel a little nervous about potential corruptions / races, would you please more thoroughly explain how you prevent all problems? > > > > > There might be other issues. > > > > Please think hard about this part. It is extremely easy to get into trouble. > > > > https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_video_track_shared.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:22: if (attributes.buffers < 0) On 2014/02/13 21:28:34, Peng wrote: > On 2014/02/13 19:23:26, yzshen1 wrote: > > I think it should be <= 0 here, right? > > buffers == 0 means using default value. Do you mean the plugin can, say, first use Configure to set buffers to a non-default value; and then later use Configure with buffer==0 to set it back to default value? Please talk about it in the API definition. An alternative is that everytime Configure is called, all attributes will be reset, and those unspecified ones will be reset to their default values. Then we don't need to support buffers==0, format==PP_VIDEOFRAME_FORMAT_UNKNOWN, etc. https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:46: int32_t OnHostMsgEnqueueBuffer(ppapi::host::HostMessageContext* context, By the way, I just realized that this method doesn't validate |index|. Moreover, MediaStreamBuffer only do DCHECKs. In a release build, a malicious IPC message can easily get us into out-of-bound array access. https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:57: // Number of buffers And trailing period. :) https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:200: void MediaStreamVideoTrackResource::OnPluginMsgConfigureReply( In the case of failure, do we guarantee that the underlying buffer configuration doesn't change? https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:125: IPC_ENUM_TRAITS(PP_VideoFrame_Format) Please use IPC_ENUM_TRAITS_MAX_VALUE
CL has been updated. PTAL. Thanks. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:211: DCHECK(frame->coded_size() == frame_size_); On 2014/02/14 18:25:43, yzshen1 wrote: > If the backend changes to support that in the future, this code may break > silently. Maybe we should fail more explicitly, for example, discard the frame, > log an error and return? > > On 2014/02/13 21:28:34, Peng wrote: > > On 2014/02/13 19:23:26, yzshen1 wrote: > > > (just to double check) So OnVideoFrame never returns frames of different > size > > or > > > format? I remember you mentioned before that it is possible to change > > > format/size in the middle? > > > > Currently, it will be always same. But w3c has an API to change the frame > size. > > But it is not supported. > How about use CHECK()? I think it can help us locate the problem in future easily. https://codereview.chromium.org/150403006/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:249: int32_t PepperMediaStreamVideoTrackHost::OnHostMsgConfigure( Please check the code in PepperMediaStreamVideoTrackResource::Configure(). 1. If plugin holds frames, xxResource::Configure() will return error. 2. If there is pending calls of configure() or getframe(), it will return error. 3. xxResource::GetFrame() will return error, if there is a pending call of Configure() is going. 1,2,3 will guarantee no frames will be used during a configure() call. So it should be very safe to re-allocate underlying buffers. https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_video_track_shared.cc (right): https://codereview.chromium.org/150403006/diff/120001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_video_track_shared.cc:22: if (attributes.buffers < 0) On 2014/02/14 18:25:43, yzshen1 wrote: > On 2014/02/13 21:28:34, Peng wrote: > > On 2014/02/13 19:23:26, yzshen1 wrote: > > > I think it should be <= 0 here, right? > > > > buffers == 0 means using default value. > > Do you mean the plugin can, say, first use Configure to set buffers to a > non-default value; and then later use Configure with buffer==0 to set it back to > default value? > Please talk about it in the API definition. > > An alternative is that everytime Configure is called, all attributes will be > reset, and those unspecified ones will be reset to their default values. Then we > don't need to support buffers==0, format==PP_VIDEOFRAME_FORMAT_UNKNOWN, etc. Yes. It provides a way to set an attribute back to the default value. I update doc of the API. Done https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_track_host_base.h (right): https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_track_host_base.h:46: int32_t OnHostMsgEnqueueBuffer(ppapi::host::HostMessageContext* context, On 2014/02/14 18:25:44, yzshen1 wrote: > By the way, I just realized that this method doesn't validate |index|. Moreover, > MediaStreamBuffer only do DCHECKs. In a release build, a malicious IPC message > can easily get us into out-of-bound array access. I changed several DCHECK to CHECK in MediaStreamBufferManager(). It will crash any compromised renderer or plugin. https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.h (right): https://codereview.chromium.org/150403006/diff/300001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.h:57: // Number of buffers On 2014/02/14 18:25:44, yzshen1 wrote: > And trailing period. :) Done. https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:200: void MediaStreamVideoTrackResource::OnPluginMsgConfigureReply( On 2014/02/14 18:25:44, yzshen1 wrote: > In the case of failure, do we guarantee that the underlying buffer configuration > doesn't change? We verify attrib_list in both plugin and renderer sides. So in most cases, if some attrib_list is invalid, the Configure() will return error directly without send IPC to renderer. So the underlying buffer will not be changed. If the IPC has been sent to renderer, the renderer should execute the configure() successfully. Otherwise must some serious error happens. For example (out of memory). In that case we can not make sure the underlying buffer is unchanged. :( https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:125: IPC_ENUM_TRAITS(PP_VideoFrame_Format) On 2014/02/14 18:25:44, yzshen1 wrote: > Please use IPC_ENUM_TRAITS_MAX_VALUE Done.
+fbarchard@chromium.org for content/renderer/pepper/DEPS, Thanks.
I have a few broader questions because some of what's going on isn't exactly clear. (Sorry, Pepper code is always very hard to follow.) https://codereview.chromium.org/150403006/diff/560001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/560001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:213: gfx::Size size = ComputeSize(source_frame_size_, plugin_frame_size_); Shouldn't we be validating somewhere around here that our buffer sizes can contain the supplied image sizes, or did I miss that? https://codereview.chromium.org/150403006/diff/560001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/560001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:45: const int32_t attrib_list[], So, are you saying attrib_list is always static data from the caller, and will never be from an untrusted source? Also, this just seems like an unnecessarily opaque and confusing implementation. Am I missing something, or would this make more sense as a vector of structures? https://codereview.chromium.org/150403006/diff/560001/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_buffer_manager.cc (right): https://codereview.chromium.org/150403006/diff/560001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_buffer_manager.cc:36: static_cast<int32_t>(sizeof(MediaStreamBuffer::Header))); I realize this is existing code, but shouldn't this be verified in release builds, or are we checking it elsewhere? https://codereview.chromium.org/150403006/diff/560001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_buffer_manager.cc:42: int32_t size = number_of_buffers_ * buffer_size; Also existing code, but where are these sizes validated and how do we know this won't overflow?
https://codereview.chromium.org/150403006/diff/560001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/560001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:213: gfx::Size size = ComputeSize(source_frame_size_, plugin_frame_size_); On 2014/02/14 22:21:25, Justin Schuh wrote: > Shouldn't we be validating somewhere around here that our buffer sizes can > contain the supplied image sizes, or did I miss that? In ppapi/shared_impl/media_stream_video_track_shared.cc, we will check the width & height provided by plugin to make sure they are less than 4096. And in InitBuffer(), if system does not have enough free memory for the buffers, the renderer will crash. WDYT? https://codereview.chromium.org/150403006/diff/560001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/560001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:45: const int32_t attrib_list[], On 2014/02/14 22:21:25, Justin Schuh wrote: > So, are you saying attrib_list is always static data from the caller, and will > never be from an untrusted source? Also, this just seems like an unnecessarily > opaque and confusing implementation. Am I missing something, or would this make > more sense as a vector of structures? 1. The arguments are from plugin. I think it is not from trusted source. 2. Because this interface is used by plugin. So we have to use c type, we can not use std::vector. 3. We do not use structure is because we want to make this API extendible. It will support more attributes in future. We would like to support more attributes in future and without break the API. WDYT? https://codereview.chromium.org/150403006/diff/560001/ppapi/shared_impl/media... File ppapi/shared_impl/media_stream_buffer_manager.cc (right): https://codereview.chromium.org/150403006/diff/560001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_buffer_manager.cc:36: static_cast<int32_t>(sizeof(MediaStreamBuffer::Header))); On 2014/02/14 22:21:25, Justin Schuh wrote: > I realize this is existing code, but shouldn't this be verified in release > builds, or are we checking it elsewhere? Those input arguments are not from third party code (plugin). The caller will make sure those arguments are valid. So we just use DCHECK() to check them in debug build. Is it necessary to use CHECK() for release build as well? https://codereview.chromium.org/150403006/diff/560001/ppapi/shared_impl/media... ppapi/shared_impl/media_stream_buffer_manager.cc:42: int32_t size = number_of_buffers_ * buffer_size; On 2014/02/14 22:21:25, Justin Schuh wrote: > Also existing code, but where are these sizes validated and how do we know this > won't overflow? Same reason. arguments are not from third party code. So we know the maximum value of them. number_of_buffers_ is in [1, 8] buffer_size is in (0, 4096 * 4096 * 4] So it should not overflow. Anyway, I will change int32_t to size_t. Done
https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:200: void MediaStreamVideoTrackResource::OnPluginMsgConfigureReply( On 2014/02/14 20:04:50, Peng wrote: > On 2014/02/14 18:25:44, yzshen1 wrote: > > In the case of failure, do we guarantee that the underlying buffer > configuration > > doesn't change? > > We verify attrib_list in both plugin and renderer sides. So in most cases, if > some attrib_list is invalid, the Configure() will return error directly without > send IPC to renderer. So the underlying buffer will not be changed. > > If the IPC has been sent to renderer, the renderer should execute the > configure() successfully. Otherwise must some serious error happens. For example > (out of memory). In that case we can not make sure the underlying buffer is > unchanged. :( > > It seems bad if we mess up the state this way. (Namely, the plugin thought that the buffer configuration is X while it is actually Y.) We should either make sure on failure the configuration of the buffer doesn't change, or terminate the track. (I think the latter is easier and safer.) https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:35: * If 0 is specified, the default value will be used. (Have you considered this?) An alternative is to reset all the fields every time Configure() is called, those fields that are not listed will be reset to default value. For example: - first Configure call: { WIDTH = 100; HEIGHT = 100; FORMAT = RGBA } - second Configure call: { FORMAT = RGBA } (width and height will be reset to default values) https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:41: * If the specified size is different with the video source (webcam), nit: "different from"? https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:51: * If the specified size is different with the video source (webcam), ditto
https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... File ppapi/proxy/media_stream_video_track_resource.cc (right): https://codereview.chromium.org/150403006/diff/300001/ppapi/proxy/media_strea... ppapi/proxy/media_stream_video_track_resource.cc:200: void MediaStreamVideoTrackResource::OnPluginMsgConfigureReply( On 2014/02/18 18:11:39, yzshen1 wrote: > On 2014/02/14 20:04:50, Peng wrote: > > On 2014/02/14 18:25:44, yzshen1 wrote: > > > In the case of failure, do we guarantee that the underlying buffer > > configuration > > > doesn't change? > > > > We verify attrib_list in both plugin and renderer sides. So in most cases, if > > some attrib_list is invalid, the Configure() will return error directly > without > > send IPC to renderer. So the underlying buffer will not be changed. > > > > If the IPC has been sent to renderer, the renderer should execute the > > configure() successfully. Otherwise must some serious error happens. For > example > > (out of memory). In that case we can not make sure the underlying buffer is > > unchanged. :( > > > > > > It seems bad if we mess up the state this way. (Namely, the plugin thought that > the buffer configuration is X while it is actually Y.) > > We should either make sure on failure the configuration of the buffer doesn't > change, or terminate the track. > (I think the latter is easier and safer.) discussed it offline. Updated the doc. Done https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:35: * If 0 is specified, the default value will be used. On 2014/02/18 18:11:40, yzshen1 wrote: > (Have you considered this?) An alternative is to reset all the fields every time > Configure() is called, those fields that are not listed will be reset to default > value. For example: > - first Configure call: { WIDTH = 100; HEIGHT = 100; FORMAT = RGBA } > - second Configure call: { FORMAT = RGBA } (width and height will be reset to > default values) I considered it. I think your suggestion and current implementation are both OK. I don't have any preference. Because it has been implemented in this way, so I did not change it. Which one do you think it is better? https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:41: * If the specified size is different with the video source (webcam), On 2014/02/18 18:11:40, yzshen1 wrote: > nit: "different from"? Done. https://codereview.chromium.org/150403006/diff/630001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:51: * If the specified size is different with the video source (webcam), On 2014/02/18 18:11:40, yzshen1 wrote: > ditto Done.
CL is updated. PTAL. Thanks
https://codereview.chromium.org/150403006/diff/860001/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/150403006/diff/860001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:35: * If this attribute is not specified or value 0 is specified for this Why do we need to have 0 as default value? It seems better to only have one way to do things. What do you think? (Here and below)
https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:52: gfx::Size ComputeSize(const gfx::Size& source, ComputeSize is not very readable. Maybe rename to GetTargetSize? https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:60: PP_VideoFrame_Format ComputeFormat(PP_VideoFrame_Format source, GetTargetFormat https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:82: media::YV12); src->format() ? https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:95: media::YV12, src->format() ? https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:113: libyuv::kFilterBox); define libyuv::kFilterBox as a const variable https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:253: gfx::Size new_size = plugin_frame_size_; Do we need to init this with plugin_frame_size_? We will set width and height right after this. https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:267: buffers_ = buffers; nit, name this buffer_num_ https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:274: if (new_format != original_format) prefer to keep consistent with what you did for size. E.g. 256 if (ComputeFormat(source_frame_format_, plugin_frame_format_) != 257 ComputeFormat(source_frame_format_, attributes.format)) { 258 changed = true; 259 } 260 plugin_frame_format_ = attributes.format; https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:282: // which are not enough to initialize buffers. wonder how should we handle the output case, i.e. how should I change my cl? Like this? if (changed && (output_mode || !source_frame_size_.IsEmpty())
CL is updated. PTAL. Thanks. https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:52: gfx::Size ComputeSize(const gfx::Size& source, On 2014/02/19 17:09:35, Ronghua Wu wrote: > ComputeSize is not very readable. Maybe rename to GetTargetSize? Done. https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:60: PP_VideoFrame_Format ComputeFormat(PP_VideoFrame_Format source, On 2014/02/19 17:09:35, Ronghua Wu wrote: > GetTargetFormat Done. https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:82: media::YV12); On 2014/02/19 17:09:35, Ronghua Wu wrote: > src->format() ? This argument is media::YUVType instead of media::VideoFrame::Type. So we need use media::YV12 https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:95: media::YV12, On 2014/02/19 17:09:35, Ronghua Wu wrote: > src->format() ? same https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:113: libyuv::kFilterBox); On 2014/02/19 17:09:35, Ronghua Wu wrote: > define libyuv::kFilterBox as a const variable Done. https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:253: gfx::Size new_size = plugin_frame_size_; On 2014/02/19 17:09:35, Ronghua Wu wrote: > Do we need to init this with plugin_frame_size_? We will set width and height > right after this. Done. https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:267: buffers_ = buffers; On 2014/02/19 17:09:35, Ronghua Wu wrote: > nit, name this buffer_num_ Done. https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:274: if (new_format != original_format) On 2014/02/19 17:09:35, Ronghua Wu wrote: > prefer to keep consistent with what you did for size. > > E.g. > 256 if (ComputeFormat(source_frame_format_, plugin_frame_format_) != > 257 ComputeFormat(source_frame_format_, attributes.format)) { > 258 changed = true; > 259 } > 260 plugin_frame_format_ = attributes.format; Done. https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:282: // which are not enough to initialize buffers. On 2014/02/19 17:09:35, Ronghua Wu wrote: > wonder how should we handle the output case, i.e. how should I change my cl? > Like this? > > if (changed && (output_mode || !source_frame_size_.IsEmpty()) Using an output_mode flag could be a solution. If we want to make code more clear, we could refactor the code a little bit. I think we could extract some common logic between input & output track to a base class, and then implement two track hosts for input & output. When we create the track, we know it is for receiving or sending frames, I think. So we could create different kind of host and associate it with the same resource implementation. WDYT? For example: class VideoTrackHostBase { bool InitBuffer(buffer, size); virtual int32_t OnConfigure(attributes) = 0; }; class VideoTrackHostForInput : VideoTrackBase, VideoSink { virtual void OnVideoFrame(frame); virtual int32_t OnHostMsgConfigure(attributes); }; class VideoTrackHostForOutput : VideoTrackBase, VideoSource { virtual int32_t OnHostMsgConfigure(attributes); }; https://codereview.chromium.org/150403006/diff/860001/ppapi/api/ppb_media_str... File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/150403006/diff/860001/ppapi/api/ppb_media_str... ppapi/api/ppb_media_stream_video_track.idl:35: * If this attribute is not specified or value 0 is specified for this On 2014/02/19 17:00:56, yzshen1 wrote: > Why do we need to have 0 as default value? It seems better to only have one way > to do things. What do you think? (Here and below) I think it is better to have a way to allow developer to set the default value explicitly. Otherwise, to set a default value, developers must configure() without specifying the attribute. It is little weird. Especially some developers prefer to put all supported attributes in the array. (For example: If I create an example code for this API, I would like to put all supported attributes in the array, and add comments to describe them.). WDYT?
lgtm https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:82: media::YV12); On 2014/02/19 19:22:36, Peng wrote: > On 2014/02/19 17:09:35, Ronghua Wu wrote: > > src->format() ? > > This argument is media::YUVType instead of media::VideoFrame::Type. So we need > use media::YV12 > Done. https://codereview.chromium.org/150403006/diff/820001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:95: media::YV12, On 2014/02/19 19:22:36, Peng wrote: > On 2014/02/19 17:09:35, Ronghua Wu wrote: > > src->format() ? > same Got it thanks. https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/860001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:282: // which are not enough to initialize buffers. On 2014/02/19 19:22:36, Peng wrote: > On 2014/02/19 17:09:35, Ronghua Wu wrote: > > wonder how should we handle the output case, i.e. how should I change my cl? > > Like this? > > > > if (changed && (output_mode || !source_frame_size_.IsEmpty()) > > Using an output_mode flag could be a solution. If we want to make code more > clear, we could refactor the code a little bit. > > I think we could extract some common logic between input & output track to a > base class, and then implement two track hosts for input & output. When we > create the track, we know it is for receiving or sending frames, I think. So we > could create different kind of host and associate it with the same resource > implementation. WDYT? > > For example: > > class VideoTrackHostBase { > bool InitBuffer(buffer, size); > virtual int32_t OnConfigure(attributes) = 0; > }; > > class VideoTrackHostForInput : VideoTrackBase, VideoSink { > virtual void OnVideoFrame(frame); > virtual int32_t OnHostMsgConfigure(attributes); > > }; > > class VideoTrackHostForOutput : VideoTrackBase, VideoSource { > virtual int32_t OnHostMsgConfigure(attributes); > }; SG https://codereview.chromium.org/150403006/diff/970001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/970001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:269: if (plugin_frame_format_ != attributes.format) { nit: remove this line or do the same for size
https://codereview.chromium.org/150403006/diff/970001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/970001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_video_track_host.cc:269: if (plugin_frame_format_ != attributes.format) { On 2014/02/19 21:16:24, Ronghua Wu wrote: > nit: remove this line or do the same for size Done.
LGTM with one nit. https://codereview.chromium.org/150403006/diff/1070001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/1070001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_video_track_host.cc:25: // Filer mode for scaling frames. filer->filter
https://codereview.chromium.org/150403006/diff/1070001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/1070001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_video_track_host.cc:25: // Filer mode for scaling frames. On 2014/02/20 17:18:42, yzshen1 wrote: > filer->filter Done.
https://codereview.chromium.org/150403006/diff/1360001/content/renderer/peppe... File content/renderer/pepper/DEPS (right): https://codereview.chromium.org/150403006/diff/1360001/content/renderer/peppe... content/renderer/pepper/DEPS:12: # TODO(penghuang): Remove it when media supports format conversion and Talked with Frank. Actually libyuv is not from third party. It is developed by Googlers. So it is safe to use it in renderer. Remove the TODO.
Okay, ipc security lgtm. It looks like the values are checked with small enough bounds that there's no need for explicit overflow checking.
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1390001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for ppapi/shared_impl/media_stream_buffer_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file ppapi/shared_impl/media_stream_buffer_manager.cc
Hunk #1 succeeded at 26 with fuzz 2 (offset -5 lines).
Hunk #2 succeeded at 34 (offset -5 lines).
Hunk #3 FAILED at 65.
1 out of 3 hunks FAILED -- saving rejects to file
ppapi/shared_impl/media_stream_buffer_manager.cc.rej
Patch: ppapi/shared_impl/media_stream_buffer_manager.cc
Index: ppapi/shared_impl/media_stream_buffer_manager.cc
diff --git a/ppapi/shared_impl/media_stream_buffer_manager.cc
b/ppapi/shared_impl/media_stream_buffer_manager.cc
index
b0588d6df4832540677c8b52fba39ac57610a762..93e1a3b03d0566f212bc6784cb2e08cdaf723223
100644
--- a/ppapi/shared_impl/media_stream_buffer_manager.cc
+++ b/ppapi/shared_impl/media_stream_buffer_manager.cc
@@ -31,7 +31,6 @@ bool MediaStreamBufferManager::SetBuffers(
scoped_ptr<base::SharedMemory> shm,
bool enqueue_all_buffers) {
DCHECK(shm);
- DCHECK(!shm_);
DCHECK_GT(number_of_buffers, 0);
DCHECK_GT(buffer_size,
static_cast<int32_t>(sizeof(MediaStreamBuffer::Header)));
@@ -40,11 +39,13 @@ bool MediaStreamBufferManager::SetBuffers(
number_of_buffers_ = number_of_buffers;
buffer_size_ = buffer_size;
- int32_t size = number_of_buffers_ * buffer_size;
+ size_t size = number_of_buffers_ * buffer_size;
shm_ = shm.Pass();
if (!shm_->Map(size))
return false;
+ buffer_queue_.clear();
+ buffers_.clear();
uint8_t* p = reinterpret_cast<uint8_t*>(shm_->memory());
for (int32_t i = 0; i < number_of_buffers; ++i) {
if (enqueue_all_buffers)
@@ -64,16 +65,16 @@ int32_t MediaStreamBufferManager::DequeueBuffer() {
}
void MediaStreamBufferManager::EnqueueBuffer(int32_t index) {
- DCHECK_GE(index, 0);
- DCHECK_LT(index, number_of_buffers_);
+ CHECK_GE(index, 0) << "Invalid buffer index";
+ CHECK_LT(index, number_of_buffers_) << "Invalid buffer index";
buffer_queue_.push_back(index);
delegate_->OnNewBufferEnqueued();
}
MediaStreamBuffer* MediaStreamBufferManager::GetBufferPointer(
int32_t index) {
- DCHECK_GE(index, 0);
- DCHECK_LT(index, number_of_buffers_);
+ CHECK_GE(index, 0) << "Invalid buffer index";
+ CHECK_LT(index, number_of_buffers_) << "Invalid buffer index";
return buffers_[index];
}
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1660001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
+dmichael for content/renderer/pepper/DEPS. Thanks.
yzshen's lgtm should be as good as mine... I think for a DEPS change, you might need an OWNER for the directory you're depending on (third_party/libyuv). I see you already have fbarchard on the reviewer list.
lgtm https://codereview.chromium.org/150403006/diff/1660001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/1660001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_video_track_host.cc:75: media::ConvertYUVToRGB32(src->data(VideoFrame::kYPlane), This all looks good, but perhaps add a todo to consider using libyuv:I420ToARGB()
https://codereview.chromium.org/150403006/diff/1660001/content/renderer/peppe... File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right): https://codereview.chromium.org/150403006/diff/1660001/content/renderer/peppe... content/renderer/pepper/pepper_media_stream_video_track_host.cc:75: media::ConvertYUVToRGB32(src->data(VideoFrame::kYPlane), On 2014/02/24 19:05:50, fbarchard wrote: > This all looks good, but perhaps add a todo to consider using > libyuv:I420ToARGB() Change it to use libyuv:I420ToARGB(). Done
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1800001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1960001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1960001
The CQ bit was checked by penghuang@chromium.org
The CQ bit was unchecked by penghuang@chromium.org
The CQ bit was checked by penghuang@chromium.org
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/150403006/1980001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1980001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1980001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1980001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1980001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1980001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1980001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by penghuang@chromium.org
The CQ bit was unchecked by penghuang@chromium.org
The CQ bit was checked by penghuang@chromium.org
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/150403006/1980001
Message was sent while issue was closed.
Change committed as 253307 |
