Issue 2307653002: Adding CastRemotingSender for media remoting.

xjz

Patchset #2 (id:20001) has been deleted

4 years, 3 months ago (2016-09-01 22:28:37 UTC) #1

xjz

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-01 22:29:10 UTC) #2

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2307653002/40001

4 years, 3 months ago (2016-09-01 22:29:48 UTC) #3

xjz

Description was changed from ========== Adding CastRemotingSender for media remoting. This Cl: 1. Adding a ...

4 years, 3 months ago (2016-09-01 22:32:04 UTC) #4

xjz

xjz@chromium.org changed reviewers: + mek@chromium.org, miu@chromium.org

4 years, 3 months ago (2016-09-01 22:32:05 UTC) #5

xjz

miu@: Addressed your comments. PTAL mek@: PTAL changes on chrome/renderer/extensions/cast_streaming_native_handler.cc. dcheng@: Need owner's approval on ...

4 years, 3 months ago (2016-09-01 22:45:11 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-01 23:14:00 UTC) #8

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/286191)

4 years, 3 months ago (2016-09-01 23:14:01 UTC) #9

xjz

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-01 23:55:00 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2307653002/60001

4 years, 3 months ago (2016-09-01 23:55:27 UTC) #11

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-02 03:34:11 UTC) #12

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-02 03:34:12 UTC) #13

miu

lgtm % nits: https://codereview.chromium.org/2307653002/diff/60001/chrome/browser/media/cast_remoting_sender.cc File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2307653002/diff/60001/chrome/browser/media/cast_remoting_sender.cc#newcode153 chrome/browser/media/cast_remoting_sender.cc:153: // media::cast::frame_sender::GetUnacknowledgedFrameCount(). s/frame_sender/FrameSender/ https://codereview.chromium.org/2307653002/diff/60001/chrome/test/data/extensions/api_test/cast_streaming/null_stream.js File chrome/test/data/extensions/api_test/cast_streaming/null_stream.js ...

4 years, 3 months ago (2016-09-02 21:32:40 UTC) #14

miu

dcheng and mek: To provide a little more context for this change: We are building ...

4 years, 3 months ago (2016-09-02 21:44:39 UTC) #15

xjz

Thanks, miu@! Addressed your comments. sky@: Need owner's approval on chrome/browser/BUILD.gn. https://codereview.chromium.org/2307653002/diff/60001/chrome/browser/media/cast_remoting_sender.cc File chrome/browser/media/cast_remoting_sender.cc (right): ...

4 years, 3 months ago (2016-09-02 22:00:11 UTC) #17

dcheng

Mostly just optional nits and some C++11 things so you don't have to write std::unique_ptr ...

4 years, 3 months ago (2016-09-10 02:31:37 UTC) #21

miu

On 2016/09/10 02:31:37, dcheng wrote: > My main question though... I looked at a bunch ...

4 years, 3 months ago (2016-09-10 23:02:08 UTC) #22

On 2016/09/10 02:31:37, dcheng wrote:
> My main question though... I looked at a bunch of cast code, and I'm pretty
sure
> I still don't understand the overall design and how the pieces fit together.
> 
> Does CastRemotingSender indirectly handle IPCs from the renderer or extension
> process? It looks like we create this object in response to an IPC, but after
> that, I'm not quite sure how it interacts with IPC.

Daniel,

If you haven't already, PTAL at my earlier message
(https://codereview.chromium.org/2307653002/#msg15). One thing I probably should
have explained better is how this CastRemotingSender is different from the
FrameSender (in src/media/cast/sender/frame_sender.h). CastRemotingSender is
basically an abbreviated implementation of FrameSender that is meant to run in
the browser process instead of the extension process.

Normally, a Cast Streaming sender works by splitting-up functionality across
both the browser and extension processes. The FrameSender in the extension
process receives raw audio or video frames, encodes them (Opus or VP8), and then
uses IPCs to pass them to the browser process. In the browser process, our
CastTransportImpl takes the frame data, breaks it up into UDP packets and sends
them over the network. The FrameSender in the extension process also handles
high-level control over the protocol (managing flow control, retransmits,
timeouts, etc.).

For our new Media Remoting project, we want to re-use Cast Streaming as a
reliable transport. However, unlike what I just described above, the data input
to a video element (e.g., src="myvideo.webm", or via MSE) in the source tab is
meant to be sent directly to a remote Cast device. There is no local playback +
tab capture + re-encode, which is a huge optimization in overall quality and
performance.

One possible design we considered was to transport the bitstream data from the
render process, to the browser process, and then forward it to the extension
process where FrameSender executes. Then FrameSender would pass the same data
(via IPCs) to the CastTransportImpl in the browser process for sending over the
network. Right away, you can probably see the huge inefficiency here: We'd be
copying the data across process boundaries four times. That's very wasteful!

So, what we've done here is create a CastRemotingSender that is meant to run in
the browser process. It will take data from a Mojo data pipe, from the source
tab in a render process, and feed it to the same CastTransportImpl for sending
over the network. With this design, we are only copying the data across process
boundaries once.

We are still using the extension to create the CastRemotingSender via the
cast.streaming APIs, since our overall design requires the Media Router
Component Extension to negotiate with the remote Cast device and synchronize the
switching-over from Tab Mirroring Cast Streaming to Media Remoting's Cast
Streaming.

BTW--A later change (https://codereview.chromium.org/2310753002/) completes the
whole picture: It shows how we will connect the renderer to this
CastRemotingSender using Mojo.

Thanks for taking the time to study all the context behind this change and
reviewing it. If you're still unsure about certain details, let's chat in front
of a whiteboard. :)

miu

Description was changed from ========== Adding CastRemotingSender for media remoting. This Cl: 1. Adding a ...

4 years, 3 months ago (2016-09-10 23:10:21 UTC) #23

dcheng

On 2016/09/10 23:02:08, miu wrote: > On 2016/09/10 02:31:37, dcheng wrote: > > My main ...

4 years, 3 months ago (2016-09-12 17:29:09 UTC) #24

On 2016/09/10 23:02:08, miu wrote:
> On 2016/09/10 02:31:37, dcheng wrote:
> > My main question though... I looked at a bunch of cast code, and I'm pretty
> sure
> > I still don't understand the overall design and how the pieces fit together.
> > 
> > Does CastRemotingSender indirectly handle IPCs from the renderer or
extension
> > process? It looks like we create this object in response to an IPC, but
after
> > that, I'm not quite sure how it interacts with IPC.
> 
> Daniel,
> 
> If you haven't already, PTAL at my earlier message
> (https://codereview.chromium.org/2307653002/#msg15). One thing I probably
should
> have explained better is how this CastRemotingSender is different from the
> FrameSender (in src/media/cast/sender/frame_sender.h). CastRemotingSender is
> basically an abbreviated implementation of FrameSender that is meant to run in
> the browser process instead of the extension process.
> 
> Normally, a Cast Streaming sender works by splitting-up functionality across
> both the browser and extension processes. The FrameSender in the extension
> process receives raw audio or video frames, encodes them (Opus or VP8), and
then
> uses IPCs to pass them to the browser process. In the browser process, our
> CastTransportImpl takes the frame data, breaks it up into UDP packets and
sends
> them over the network. The FrameSender in the extension process also handles
> high-level control over the protocol (managing flow control, retransmits,
> timeouts, etc.).
> 
> For our new Media Remoting project, we want to re-use Cast Streaming as a
> reliable transport. However, unlike what I just described above, the data
input
> to a video element (e.g., src="myvideo.webm", or via MSE) in the source tab is
> meant to be sent directly to a remote Cast device. There is no local playback
+
> tab capture + re-encode, which is a huge optimization in overall quality and
> performance.
> 
> One possible design we considered was to transport the bitstream data from the
> render process, to the browser process, and then forward it to the extension
> process where FrameSender executes. Then FrameSender would pass the same data
> (via IPCs) to the CastTransportImpl in the browser process for sending over
the
> network. Right away, you can probably see the huge inefficiency here: We'd be
> copying the data across process boundaries four times. That's very wasteful!
> 
> So, what we've done here is create a CastRemotingSender that is meant to run
in
> the browser process. It will take data from a Mojo data pipe, from the source
> tab in a render process, and feed it to the same CastTransportImpl for sending
> over the network. With this design, we are only copying the data across
process
> boundaries once.
> 
> We are still using the extension to create the CastRemotingSender via the
> cast.streaming APIs, since our overall design requires the Media Router
> Component Extension to negotiate with the remote Cast device and synchronize
the
> switching-over from Tab Mirroring Cast Streaming to Media Remoting's Cast
> Streaming.
> 
> BTW--A later change (https://codereview.chromium.org/2310753002/) completes
the
> whole picture: It shows how we will connect the renderer to this
> CastRemotingSender using Mojo.
> 
> Thanks for taking the time to study all the context behind this change and
> reviewing it. If you're still unsure about certain details, let's chat in
front
> of a whiteboard. :)

Hi miu@

I do understand the high-level concepts behind this change (we want to avoid
excessive process hops), but I'm just trying to understand exactly how it ends
up mapping to code. In particular, ChromeRemotingSender has a bit of a state
machine around timestamps and frames, and I'm trying to understand how much, if
any, of that is driven by a renderer.

xjz

Addressed dcheng's comments. PTAL https://codereview.chromium.org/2307653002/diff/80001/chrome/browser/media/cast_remoting_sender.cc File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2307653002/diff/80001/chrome/browser/media/cast_remoting_sender.cc#newcode42 chrome/browser/media/cast_remoting_sender.cc:42: RemotingRtcpClient(base::WeakPtr<CastRemotingSender> remoting_sender) On 2016/09/10 02:31:36, ...

4 years, 3 months ago (2016-09-12 18:22:06 UTC) #25

miu

On 2016/09/12 17:29:09, dcheng wrote: > Hi miu@ > > I do understand the high-level ...

4 years, 3 months ago (2016-09-12 19:36:53 UTC) #26

dcheng

On 2016/09/12 19:36:53, miu wrote: > On 2016/09/12 17:29:09, dcheng wrote: > > Hi miu@ ...

4 years, 3 months ago (2016-09-13 01:22:21 UTC) #27

On 2016/09/12 19:36:53, miu wrote:
> On 2016/09/12 17:29:09, dcheng wrote:
> > Hi miu@
> > 
> > I do understand the high-level concepts behind this change (we want to avoid
> > excessive process hops), but I'm just trying to understand exactly how it
ends
> > up mapping to code. In particular, ChromeRemotingSender has a bit of a state
> > machine around timestamps and frames, and I'm trying to understand how much,
> if
> > any, of that is driven by a renderer.
> 
> None of that state comes from the renderer, and the renderer is not provided
any
> of that state in any form. The timestamps and other state are completely
managed
> by CastRemotingSender for protocol-level concerns, such as: 1) measuring the
> network link round-trip-time; 2) when to expect ACKs from the remote peer; 3)
> deciding when to re-send "kickstart" packets to overcome severe packet loss
> scenarios; 4) logging events, which we use to diagnose problems in-the-wild.
> 
> None of this is new functionality. It was brought-over from FrameSender and
then
> simplified for this use case. As a side-point: We would have liked to have
> factored much of this in a shared base class, but DEPS rules here are already
> rather complex; and we didn't want to just allow all media/cast code to
suddenly
> be valid for use in the browser process where we don't have sandbox
protection.
> ;-)

I understand that this is functionality that is already written, but my
understanding is that FrameSender formerly only lived in the renderer, hence the
extra scrutiny and trying to understand exactly how the state is driven.

In any case, LGTM from a security perspective.

https://codereview.chromium.org/2307653002/diff/80001/chrome/browser/media/ca...
File chrome/browser/media/cast_remoting_sender.cc (right):

https://codereview.chromium.org/2307653002/diff/80001/chrome/browser/media/ca...
chrome/browser/media/cast_remoting_sender.cc:82:
last_sent_frame_id_(media::cast::FrameId::first() - 1),
On 2016/09/12 18:22:05, xjz wrote:
> On 2016/09/10 02:31:35, dcheng wrote:
> > Seems like it'd be better to have media::Cast::FrameId::invalid_id() or
> > something (this shows up in several other places as well)
> 
> |last_sent_frame_id_| is initialized to this value so that we can still use
> |last_sent_frame_id_ + 1| to get the FrameId of the first frame.

It doesn't have to be done in this CL, but since it's not the first time this
construct has shown up, it might be nice to give it an explicit name of some
sort. Otherwise, a reviewer like me (who's not familiar with cast) has to go
look at FrameId() and convince myself that this is, in fact, an OK thing to do
=)

https://codereview.chromium.org/2307653002/diff/100001/chrome/browser/media/c...
File chrome/browser/media/cast_remoting_sender.cc (right):

https://codereview.chromium.org/2307653002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_sender.cc:328: // Force to clear internal
memory.
Nit: remove this comment, since I think STLClearObject() should make that clear.

xjz

Addressed dcheng's comments. Thanks for reviewing! https://codereview.chromium.org/2307653002/diff/80001/chrome/browser/media/cast_remoting_sender.cc File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2307653002/diff/80001/chrome/browser/media/cast_remoting_sender.cc#newcode82 chrome/browser/media/cast_remoting_sender.cc:82: last_sent_frame_id_(media::cast::FrameId::first() - 1), ...

4 years, 3 months ago (2016-09-13 17:06:10 UTC) #28

xjz

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, mek@chromium.org, sky@chromium.org, dcheng@chromium.org ...

4 years, 3 months ago (2016-09-13 17:06:43 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2307653002/120001

4 years, 3 months ago (2016-09-13 17:07:02 UTC) #31

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-13 17:09:37 UTC) #32

commit-bot: I haz the power

Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/68240) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, ...

4 years, 3 months ago (2016-09-13 17:09:39 UTC) #33

xjz

The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org, miu@chromium.org, dcheng@chromium.org, sky@chromium.org ...

4 years, 3 months ago (2016-09-13 17:50:10 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2307653002/140001

4 years, 3 months ago (2016-09-13 17:50:38 UTC) #36

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-13 20:51:59 UTC) #37

commit-bot: I haz the power

Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/292234)

4 years, 3 months ago (2016-09-13 20:52:01 UTC) #38

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2307653002/140001

4 years, 3 months ago (2016-09-13 21:28:42 UTC) #40

commit-bot: I haz the power

Description was changed from ========== Adding CastRemotingSender for media remoting. This Cl: 1. Adding a ...

4 years, 3 months ago (2016-09-13 23:30:03 UTC) #41

commit-bot: I haz the power

Description was changed from ========== Adding CastRemotingSender for media remoting. This Cl: 1. Adding a ...

4 years, 3 months ago (2016-09-13 23:31:57 UTC) #43

commit-bot: I haz the power

Patchset 7 (id:??) landed as https://crrev.com/7bffeb193c34e56437778094f847ec65e90deaea Cr-Commit-Position: refs/heads/master@{#418409}

4 years, 3 months ago (2016-09-13 23:31:59 UTC) #44

Jeffrey Yasskin

4 years, 3 months ago (2016-09-15 20:48:12 UTC) #45

Message was sent while issue was closed.

I'll send you a patch.

https://codereview.chromium.org/2307653002/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_config.h (right):

https://codereview.chromium.org/2307653002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_config.h:69: int32_t rtp_stream_id;
You forgot to initialize this field in the constructor, causing the MSan error
at
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes...:

Uninitialized bytes in __msan_check_mem_is_initialized at offset 0 inside
[0x7fbbe5ccde80, 4)
==1==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xaf3e2ba in WriteBytesCommon base/pickle.cc:478:3
    #1 0xaf3e2ba in WriteBytesStatic<4> base/pickle.cc:443:0
    #2 0xf7f518f in WritePOD<int> base/pickle.h:367:5
    #3 0xf7f518f in WriteInt base/pickle.h:229:0
    #4 0xf7f518f in Write ipc/ipc_message_utils.h:177:0
    #5 0xf7f518f in WriteParam<int> ipc/ipc_message_utils.h:104:0
    #6 0xf7f518f in Write chrome/common/cast_messages.h:85:0
    #7 0xf7dee43 in WriteParam<media::cast::CastTransportRtpConfig>
ipc/ipc_message_utils.h:104:3
    #8 0xf7dee43 in Write ipc/ipc_message_utils.h:711:0
    #9 0xf7dee43 in WriteParam<std::__1::tuple<const int &, const
media::cast::CastTransportRtpConfig &> > ipc/ipc_message_utils.h:104:0
    #10 0xf7dee43 in MessageT ipc/ipc_message_templates_impl.h:29:0
    #11 0xc4ac6e6 in MessageT<true, false> ipc/ipc_message_templates.h:99:33
    #12 0xc4ac6e6 in InitializeStream
chrome/renderer/media/cast_transport_ipc.cc:48:0
    #13 0xad6d53d in FrameSender media/cast/sender/frame_sender.cc:97:21
    #14 0xad487bf in AudioSender media/cast/sender/audio_sender.cc:23:7
    #15 0xad3a9e4 in InitializeAudio media/cast/cast_sender_impl.cc:120:11
    #16 0xc4a6031 in StartAudio
chrome/renderer/media/cast_session_delegate.cc:127:17
    #17 0xb0e7e7d in Run base/callback.h:64:12
    #18 0xb0e7e7d in RunTask base/debug/task_annotator.cc:54:0
    #19 0xaece7ab in RunTask base/message_loop/message_loop.cc:488:19
    #20 0xaed02ea in DeferOrRunPendingTask
base/message_loop/message_loop.cc:497:5
    #21 0xaed1e6b in DoWork base/message_loop/message_loop.cc:621:13
    #22 0xaee2613 in Run base/message_loop/message_pump_libevent.cc:217:31
    #23 0xaf69a1c in Run base/run_loop.cc:35:10
    #24 0xb0110ac in ThreadMain base/threading/thread.cc:307:3
    #25 0xaffc645 in ThreadFunc base/threading/platform_thread_posix.cc:71:13
    #26 0x7fbbf2b46e99 in start_thread
/build/eglibc-oqps9y/eglibc-2.15/nptl/pthread_create.c:308:0
    #27 0x7fbbebf9636c in ??
/build/eglibc-oqps9y/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112:0

  Uninitialized value was stored to memory at
    #0 0xf7f54c1 in Write ipc/ipc_message_utils.h:177:64
    #1 0xf7f54c1 in WriteParam<int> ipc/ipc_message_utils.h:104:0
    #2 0xf7f54c1 in Write chrome/common/cast_messages.h:85:0
    #3 0xf7dee43 in WriteParam<media::cast::CastTransportRtpConfig>
ipc/ipc_message_utils.h:104:3
    #4 0xf7dee43 in Write ipc/ipc_message_utils.h:711:0
    #5 0xf7dee43 in WriteParam<std::__1::tuple<const int &, const
media::cast::CastTransportRtpConfig &> > ipc/ipc_message_utils.h:104:0
    #6 0xf7dee43 in MessageT ipc/ipc_message_templates_impl.h:29:0
    #7 0xc4ac6e6 in MessageT<true, false> ipc/ipc_message_templates.h:99:33
    #8 0xc4ac6e6 in InitializeStream
chrome/renderer/media/cast_transport_ipc.cc:48:0
    #9 0xad6d53d in FrameSender media/cast/sender/frame_sender.cc:97:21
    #10 0xad487bf in AudioSender media/cast/sender/audio_sender.cc:23:7
    #11 0xad3a9e4 in InitializeAudio media/cast/cast_sender_impl.cc:120:11
    #12 0xc4a6031 in StartAudio
chrome/renderer/media/cast_session_delegate.cc:127:17
    #13 0xb0e7e7d in Run base/callback.h:64:12
    #14 0xb0e7e7d in RunTask base/debug/task_annotator.cc:54:0
    #15 0xaece7ab in RunTask base/message_loop/message_loop.cc:488:19
    #16 0xaed02ea in DeferOrRunPendingTask
base/message_loop/message_loop.cc:497:5
    #17 0xaed1e6b in DoWork base/message_loop/message_loop.cc:621:13
    #18 0xaee2613 in Run base/message_loop/message_pump_libevent.cc:217:31
    #19 0xaf69a1c in Run base/run_loop.cc:35:10
    #20 0xb0110ac in ThreadMain base/threading/thread.cc:307:3
    #21 0xaffc645 in ThreadFunc base/threading/platform_thread_posix.cc:71:13
    #22 0x7fbbf2b46e99 in start_thread
/build/eglibc-oqps9y/eglibc-2.15/nptl/pthread_create.c:308:0

  Uninitialized value was created by an allocation of 'transport_config' in the
stack frame of function
'_ZN5media4cast11FrameSenderC2E13scoped_refptrINS0_15CastEnvironmentEEPNS0_13CastTransportERKNS0_17FrameSenderConfigEPNS0_17CongestionControlE'
    #0 0xad6c6e0 in FrameSender media/cast/sender/frame_sender.cc:78:0

SUMMARY: MemorySanitizer: use-of-uninitialized-value
(/b/swarm_slave/w/iriJwbdy/out/Release/browser_tests+0xaf3e2ba)

Issue 2307653002: Adding CastRemotingSender for media remoting. (Closed)

Description

Patch Set 1 : Final patch in the origial WIP CL. #

Patch Set 2 : Addressed miu's comments. #

Patch Set 3 : Fix. #

Patch Set 4 : Addressed miu's comments. #

Patch Set 5 : Addressed dcheng's comments. #

Patch Set 6 : Addressed dcheng's comments. #

Patch Set 7 : Rebased. #

Messages