Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14968002:
Glue code to connect PPAPI proxy to MediaStream sources and destinations. (Closed)
|
Created:
7 years, 7 months ago by bbudge Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGlue code to connect PPAPI proxy to MediaStream sources and destinations.
This is a bit rough but putting it up for review to make schedule.
BUG=230980
R=ronghuawu@chromium.org, tsepez@chromium.org, yzshen@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198576
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Add lock. #
Total comments: 27
Patch Set 3 : Post a task to receive frame. Close in dtor. Other fixes. #Patch Set 4 : Fix casts. Use base::Time for timestamp calculation.x #
Total comments: 5
Patch Set 5 : Address Yuzhu's comments. #Patch Set 6 : Clarify time calculations. #Patch Set 7 : Fix time delta calculation which was correct before. #
Messages
Total messages: 16 (0 generated)
tsepez for security review (*_host.cc Open methods) juberti2, ronghua for correctness of pixel / timestamp conversion raymes, yuzhu for overall proxy.
Thanks Bill! https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_destination_host.cc:55: stream_url_ = gurl.spec(); stream_url_ doesn't seem be used anywhere else outside this function https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_destination_host.cc:90: static_cast<int64_t>(timestamp) * talk_base::kNumNanosecsPerSec; Can you confirm the |timestamp|'s unit is second? Seems be huge for time stamp. If this is actually the case, maybe we should just use the current time as the timestamp. https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_source_host.cc:58: last_frame_.reset(frame); This callback will be called from the libjingle workerthread. And I saw you aslo access |last_frame_| in OnHostMsgGetFrame, which is another thread. So I think you need some protection.
https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_destination_host.cc:55: stream_url_ = gurl.spec(); On 2013/05/05 15:39:35, Ronghua Wu wrote: > stream_url_ doesn't seem be used anywhere else outside this function Done. I removed it since it isn't needed after opening. https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_destination_host.cc:90: static_cast<int64_t>(timestamp) * talk_base::kNumNanosecsPerSec; On 2013/05/05 15:39:35, Ronghua Wu wrote: > Can you confirm the |timestamp|'s unit is second? Seems be huge for time stamp. > > If this is actually the case, maybe we should just use the current time as the > timestamp. The value is in seconds, but held in a double so fractions of a second are possible. It is awkward but matches the time unit used for input events from Webkit. https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_source_host.cc:58: last_frame_.reset(frame); On 2013/05/05 15:39:35, Ronghua Wu wrote: > This callback will be called from the libjingle workerthread. And I saw you aslo > access |last_frame_| in OnHostMsgGetFrame, which is another thread. So I think > you need some protection. Done. I added a lock. It wasn't clear from comments what threads were involved.
https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_destination_host.cc:90: static_cast<int64_t>(timestamp) * talk_base::kNumNanosecsPerSec; On 2013/05/06 04:02:15, bbudge1 wrote: > On 2013/05/05 15:39:35, Ronghua Wu wrote: > > Can you confirm the |timestamp|'s unit is second? Seems be huge for time > stamp. > > > > If this is actually the case, maybe we should just use the current time as the > > timestamp. > > The value is in seconds, but held in a double so fractions of a second are > possible. It is awkward but matches the time unit used for input events from > Webkit. Then you probably want to do the cast after convert it to nonsec. https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_source_host.cc:58: last_frame_.reset(frame); On 2013/05/06 04:02:15, bbudge1 wrote: > On 2013/05/05 15:39:35, Ronghua Wu wrote: > > This callback will be called from the libjingle workerthread. And I saw you > aslo > > access |last_frame_| in OnHostMsgGetFrame, which is another thread. So I think > > you need some protection. > > Done. I added a lock. It wasn't clear from comments what threads were involved. Thanks. My bad. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:158: get_frame_pending_ = true; You need to protect get_frame_pending_ as well. Move the lock to the beginning of the method.
Haven't looked at the details but a couple of high-level comments. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:58: base::AutoLock lock(lock_); Usually posting tasks is preferred over locks if it's possible. Take a look at http://www.chromium.org/developers/design-documents/threading. In this case, you might post a task to the main thread with the frame pointer. You can cache the current MessageLoopProxy on construction (see https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...) https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:178: source_handler_->Close(stream_url_, this); Do we need to worry about the case where the resource is destroyed before Close() is called? Should this be run on destruction as well?
https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:9: #include "content/renderer/render_thread_impl.h" Is this needed? https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:64: int32_t result = ConvertFrame(&image_data_resource, ×tamp); Since we always call ConvertFrame and then SendFrame, could we just tie the two functions together (e.g. ConvertAndSendFrame()?). Then we wouldn't have to have the out paramaters. It could just have a return value indicating if the frame was successfully converted and sent. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:119: talk_base::kNumNanosecsPerSec; nit: 4 space indent https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:153: HostMessageContext* context) { Should you check that Open() was successful before allowing this to succeed? Or maybe it doesn't matter.
https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_destination_host.cc:89: static_cast<int64_t>(timestamp) * talk_base::kNumNanosecsPerSec; nit: Shall we use Time::kNanosecondsPerSecond in base/time? Is there a reason to use talk_base here? https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:71: int32_t PepperVideoSourceHost::ConvertFrame( We are not supposed to manipulate resource from a different thread. In the renderer, we are running on a single thread. (For example, the resource tracker is "SINGLE_THREADED".) That is the main reason why we should post a task to the main thread. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:128: host()->SendReply( host is not guaranteed to be thread safe as well. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:142: stream_url_ = gurl.spec(); nit: better to move it below the failure return at line 144. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.h (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.h:58: base::Lock lock_; I agree with the comment that we should use PostTask instead of Lock if possible. If we finally decide to use Lock, please comment on what members are protected by this lock, and if some members are not protected, which thread can access them.
Flow looks ok, just a couple of casts to check. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_destination_host.cc:89: static_cast<int64_t>(timestamp) * talk_base::kNumNanosecsPerSec; How do we know what the epoch is for PP_Timeticks and whether this will overflow? https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:76: int32_t height = static_cast<int32_t>(frame->GetHeight()); How do we know these don't overflow when you cast a size_t to int32_t?
Post a task to receive frame from media layer. Close VideoSourceHandler in dtor, check it's opened before getting frame or closing. Fix casts and use base::Time for timestamp conversion. https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/14968002/diff/2001/content/renderer/pepper/pe... content/renderer/pepper/pepper_video_destination_host.cc:90: static_cast<int64_t>(timestamp) * talk_base::kNumNanosecsPerSec; On 2013/05/06 04:42:07, Ronghua Wu wrote: > On 2013/05/06 04:02:15, bbudge1 wrote: > > On 2013/05/05 15:39:35, Ronghua Wu wrote: > > > Can you confirm the |timestamp|'s unit is second? Seems be huge for time > > stamp. > > > > > > If this is actually the case, maybe we should just use the current time as > the > > > timestamp. > > > > The value is in seconds, but held in a double so fractions of a second are > > possible. It is awkward but matches the time unit used for input events from > > Webkit. > > Then you probably want to do the cast after convert it to nonsec. Good catch. Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_destination_host.cc:89: static_cast<int64_t>(timestamp) * talk_base::kNumNanosecsPerSec; On 2013/05/06 16:47:28, yzshen1 wrote: > nit: Shall we use Time::kNanosecondsPerSecond in base/time? > Is there a reason to use talk_base here? Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.cc (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:9: #include "content/renderer/render_thread_impl.h" Not any more. Done. On 2013/05/06 16:00:11, raymes1 wrote: > Is this needed? https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:58: base::AutoLock lock(lock_); Yes, I grepped and didn't find any resource using locks. Switched to posting a task. Done. On 2013/05/06 05:11:33, raymes wrote: > Usually posting tasks is preferred over locks if it's possible. Take a look at > http://www.chromium.org/developers/design-documents/threading. In this case, you > might post a task to the main thread with the frame pointer. You can cache the > current MessageLoopProxy on construction (see > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...) https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:64: int32_t result = ConvertFrame(&image_data_resource, ×tamp); The reason I split these is that there were a lot of 'if's with early outs returning error codes. Without a separate function, I would have to reverse the sense of these and get deep nesting. Also, in GotFrame, we always send the reply, but in OnGetFrame we return just an error code on failure. So this actually turned out to be cleaner. On 2013/05/06 16:00:11, raymes1 wrote: > Since we always call ConvertFrame and then SendFrame, could we just tie the two > functions together (e.g. ConvertAndSendFrame()?). Then we wouldn't have to have > the out paramaters. It could just have a return value indicating if the frame > was successfully converted and sent. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:71: int32_t PepperVideoSourceHost::ConvertFrame( On 2013/05/06 16:47:28, yzshen1 wrote: > We are not supposed to manipulate resource from a different thread. In the > renderer, we are running on a single thread. > (For example, the resource tracker is "SINGLE_THREADED".) > > That is the main reason why we should post a task to the main thread. Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:76: int32_t height = static_cast<int32_t>(frame->GetHeight()); I'm assuming they're relatively small integers. I'll use checked casts. Done. On 2013/05/06 17:19:35, Tom Sepez wrote: > How do we know these don't overflow when you cast a size_t to int32_t? https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:119: talk_base::kNumNanosecsPerSec; On 2013/05/06 16:00:11, raymes1 wrote: > nit: 4 space indent Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:128: host()->SendReply( On 2013/05/06 16:47:28, yzshen1 wrote: > host is not guaranteed to be thread safe as well. Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:142: stream_url_ = gurl.spec(); On 2013/05/06 16:47:28, yzshen1 wrote: > nit: better to move it below the failure return at line 144. Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:153: HostMessageContext* context) { On 2013/05/06 16:00:11, raymes1 wrote: > Should you check that Open() was successful before allowing this to succeed? Or > maybe it doesn't matter. Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:158: get_frame_pending_ = true; On 2013/05/06 04:42:07, Ronghua Wu wrote: > You need to protect get_frame_pending_ as well. Move the lock to the beginning > of the method. Done. https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.cc:178: source_handler_->Close(stream_url_, this); I'm not sure whether the deletion of VideoSourceHandler closes, so I added a Close method and call it from the destructor. On 2013/05/06 05:11:33, raymes wrote: > Do we need to worry about the case where the resource is destroyed before > Close() is called? Should this be run on destruction as well? https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.h (right): https://codereview.chromium.org/14968002/diff/10001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.h:58: base::Lock lock_; On 2013/05/06 16:47:28, yzshen1 wrote: > I agree with the comment that we should use PostTask instead of Lock if > possible. If we finally decide to use Lock, please comment on what members are > protected by this lock, and if some members are not protected, which thread can > access them. Done. Used a task.
LGTM with two nits. Thanks! https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_destination_host.h (right): https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_destination_host.h:11: #include "base/time.h" nit: please move it to .cc since the header doesn't need it. https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.h (right): https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.h:58: base::MessageLoopProxy* main_message_loop_proxy_; shall we use scoped_refptr<>?
https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_destination_host.h (right): https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_destination_host.h:11: #include "base/time.h" On 2013/05/06 20:40:18, yzshen1 wrote: > nit: please move it to .cc since the header doesn't need it. Done. https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... File content/renderer/pepper/pepper_video_source_host.h (right): https://codereview.chromium.org/14968002/diff/27001/content/renderer/pepper/p... content/renderer/pepper/pepper_video_source_host.h:58: base::MessageLoopProxy* main_message_loop_proxy_; Done. I copied this from content/renderer/pepper/pepper_platform_audio_input_impl.* Should we also change that (in another CL of course)? On 2013/05/06 20:40:18, yzshen1 wrote: > shall we use scoped_refptr<>?
LGTM thanks!
LGTM
still LGTM https://chromiumcodereview.appspot.com/14968002/diff/27001/content/renderer/p... File content/renderer/pepper/pepper_video_source_host.h (right): https://chromiumcodereview.appspot.com/14968002/diff/27001/content/renderer/p... content/renderer/pepper/pepper_video_source_host.h:58: base::MessageLoopProxy* main_message_loop_proxy_; Yeah. It makes sense to me to also change it. (in a separate CL) On 2013/05/06 20:52:01, bbudge1 wrote: > Done. I copied this from > content/renderer/pepper/pepper_platform_audio_input_impl.* > > Should we also change that (in another CL of course)? > > On 2013/05/06 20:40:18, yzshen1 wrote: > > shall we use scoped_refptr<>? >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/14968002/43001
Message was sent while issue was closed.
Committed patchset #7 manually as r198576 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
