content/common/media/video_capture_messages.h - Issue 1873293002: Report if video capturing meets output protection requirement.

Unified Diff: content/common/media/video_capture_messages.h

Issue 1873293002: Report if video capturing meets output protection requirement. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Addressed comments. Rebased. Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« content/browser/renderer_host/media/media_stream_manager.cc ('K') | « content/browser/renderer_host/media/video_capture_host.cc ('k') | content/content_renderer.gypi » ('j') | content/public/browser/media_observer.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/common/media/video_capture_messages.h

diff --git a/content/common/media/video_capture_messages.h b/content/common/media/video_capture_messages.h

index 5241898beb2da74a58d5ee00e46fb02a4f231f82..30347a3244d8c169d8cc3f98900c9eeb76e2c0f4 100644

--- a/content/common/media/video_capture_messages.h

+++ b/content/common/media/video_capture_messages.h

@@ -6,6 +6,7 @@

#include "content/common/content_export.h"

#include "content/common/media/video_capture.h"

#include "content/public/common/common_param_traits.h"

+#include "content/public/common/media_stream_request.h"

#include "ipc/ipc_message_macros.h"

#include "media/base/video_capture_types.h"

#include "media/base/video_frame.h"

@@ -116,6 +117,13 @@ IPC_MESSAGE_CONTROL1(VideoCaptureHostMsg_RequestRefreshFrame,

IPC_MESSAGE_CONTROL1(VideoCaptureHostMsg_Stop,

int /* device_id */)

+// Tell the browser process if the video capture is secure (i.e., all

+// connected video sinks meet the requirement of output protection.).

+IPC_MESSAGE_CONTROL3(VideoCaptureHostMsg_SetCapturingLinkSecured,

miu 2016/04/26 01:25:13 Hmm...It seems like this should be a MediaStreamHo

xjz 2016/04/29 00:11:42 The reason to use MediaStreamType here is that the

miu 2016/05/02 21:02:45 I should have explained more clearly before: What

I should have explained more clearly before: What I'm concerned about is the encapsulation of responsibilities/functionality here. MediaStreamManager is responsible for managing control and permissions for the streams (and bubbling-up metadata/state to the embedder), whereas VideoCaptureHost is only supposed to be responsible for delivering video frames across browser-->renderer process. More specifically: 1. The code you added in VideoCaptureHost has to call into MediaStreamManager directly anyway. In other words, VideoCaptureHost doesn't seem to need to be involved in propagating this signal, but MediaStreamManager does. 2. The MediaObserver, which you use to propagate the secure link signal to the embedder (i.e., the code in src/chrome/...), is something only MediaStreamManager knows about. 3. The secure link state is "control/permissions metadata management," and not related to video frame delivery. MediaStreamManager seems to be a more appropriate handler for this information. It should be relatively simple to migrate the IPC messaging in this change: 1. Just rename VideoCaptureHostMsg_SetCapturingLinkSecured to MediaStreamHostMsg_SetCapturingLinkSecured, and put it in media_stream_messages.h; and the use of MediaStreamType+session_id would be reasonable here. 2. Have MediaStreamDispatcherHost handle the IPC message (browser-side) rather than VideoCaptureHost. 3. LocalVideoCapturerSource (in the render process) can send the IPC to the browser process via MediaStreamDispatcher: First, you'll need to plumb-in access to the RenderFrameImpl instance from the LocalVideoCapturerSource by passing render_frame() in the constructor call from here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... LocalVideoCapturerSource should extend RenderFrameObserver to always be sure the pointer to the RenderFrameImpl is valid before trying to call: if (render_frame()) render_frame()->GetMediaStreamDispatcher()->Send(new MediaStreamHostMsg_...) If this is a bit confusing, feel free to stop by my desk, and I can explain how RenderFrameImpl works in more detail.

xjz 2016/05/04 18:23:48 Thanks for the clear explanation! Changed the IPC

On 2016/05/02 21:02:45, miu wrote: > On 2016/04/29 00:11:42, xjz wrote: > > On 2016/04/26 01:25:13, miu wrote: > > > Hmm...It seems like this should be a MediaStreamHostMsg instead (in > > > content/common/media/media_stream_messages.h). Having VideoCaptureHost > worry > > > about MediaStreamTypes feels like an abstraction violation. > > > > The reason to use MediaStreamType here is that the |session_id| is unique only > > for the same MediaStreamType. Now changed to use device id, which should also > > work. > > I should have explained more clearly before: What I'm concerned about is the > encapsulation of responsibilities/functionality here. MediaStreamManager is > responsible for managing control and permissions for the streams (and > bubbling-up metadata/state to the embedder), whereas VideoCaptureHost is only > supposed to be responsible for delivering video frames across browser-->renderer > process. More specifically: > > 1. The code you added in VideoCaptureHost has to call into MediaStreamManager > directly anyway. In other words, VideoCaptureHost doesn't seem to need to be > involved in propagating this signal, but MediaStreamManager does. > > 2. The MediaObserver, which you use to propagate the secure link signal to the > embedder (i.e., the code in src/chrome/...), is something only > MediaStreamManager knows about. > > 3. The secure link state is "control/permissions metadata management," and not > related to video frame delivery. MediaStreamManager seems to be a more > appropriate handler for this information. > > It should be relatively simple to migrate the IPC messaging in this change: > > 1. Just rename VideoCaptureHostMsg_SetCapturingLinkSecured to > MediaStreamHostMsg_SetCapturingLinkSecured, and put it in > media_stream_messages.h; and the use of MediaStreamType+session_id would be > reasonable here. > > 2. Have MediaStreamDispatcherHost handle the IPC message (browser-side) rather > than VideoCaptureHost. > > 3. LocalVideoCapturerSource (in the render process) can send the IPC to the > browser process via MediaStreamDispatcher: First, you'll need to plumb-in access > to the RenderFrameImpl instance from the LocalVideoCapturerSource by passing > render_frame() in the constructor call from here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > LocalVideoCapturerSource should extend RenderFrameObserver to always be sure the > pointer to the RenderFrameImpl is valid before trying to call: if > (render_frame()) render_frame()->GetMediaStreamDispatcher()->Send(new > MediaStreamHostMsg_...) If this is a bit confusing, feel free to stop by my > desk, and I can explain how RenderFrameImpl works in more detail.

Thanks for the clear explanation! Changed the IPC message to MediaStreamHostMsg.

+ content::MediaStreamType, /* type */

+ int, /* session_id */

+ bool /* is_secure */)

// Tell the browser process that the renderer has finished reading from

// a buffer previously delivered by VideoCaptureMsg_BufferReady.

IPC_MESSAGE_CONTROL4(VideoCaptureHostMsg_BufferReady,