Interfaces for encoded video sources

content/common/media/encoded_video_source_messages.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/common/content_export.h"
+#include "ipc/ipc_message_macros.h"
+#include "ipc/ipc_message_start.h"
+#include "media/video/video_encode_types.h"
+
+#undef IPC_MESSAGE_EXPORT
+#define IPC_MESSAGE_EXPORT CONTENT_EXPORT
+#define IPC_MESSAGE_START EncodedVideoSourceMsgStart

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

I thought you were going to merge into this CL the code that uses these
messages.  Both in order to settle otherwise-theoretical debates, and per this
comment from the other CL: 

On 2013/03/06 23:35:50, Cris Neckar wrote:

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

I agreed offline with Cris that I'll add just empty stubs for now, which I did
now in patch, and include security guys in any follow-up reviews that add the
handlers for these messages.

+
+IPC_ENUM_TRAITS(media::VideoCodec)
+
+IPC_STRUCT_TRAITS_BEGIN(media::TemporalLayerParameters)
+  IPC_STRUCT_TRAITS_MEMBER(enabled)
+  IPC_STRUCT_TRAITS_MEMBER(target_bitrate)
+IPC_STRUCT_TRAITS_END()
+
+IPC_STRUCT_TRAITS_BEGIN(media::RuntimeVideoEncodingParameters)
+  IPC_STRUCT_TRAITS_MEMBER(frames_per_second)
+  IPC_STRUCT_TRAITS_MEMBER(max_bitrate)
+  IPC_STRUCT_TRAITS_MEMBER(max_qp)
+  IPC_STRUCT_TRAITS_MEMBER(temporal_layer_params)
+IPC_STRUCT_TRAITS_END()
+
+IPC_STRUCT_TRAITS_BEGIN(media::VideoEncodingParameters)
+  IPC_STRUCT_TRAITS_MEMBER(codec)
+  IPC_STRUCT_TRAITS_MEMBER(resolution)
+  IPC_STRUCT_TRAITS_MEMBER(temporal_layer_count)
+  IPC_STRUCT_TRAITS_MEMBER(runtime_params)
+IPC_STRUCT_TRAITS_END()
+
+IPC_STRUCT_TRAITS_BEGIN(media::BufferEncodingMetadata)
+  IPC_STRUCT_TRAITS_MEMBER(timestamp)
+  IPC_STRUCT_TRAITS_MEMBER(frame_type_flags)
+  IPC_STRUCT_TRAITS_MEMBER(temporal_layer_id)
+  IPC_STRUCT_TRAITS_MEMBER(layer_sync)
+  IPC_STRUCT_TRAITS_MEMBER(droppable)
+IPC_STRUCT_TRAITS_END()
+
+// Message from Renderer to Browser process to add a bitstream with specific
+// parameters. A successful request results in beginning of streaming and
+// EncoderVideoSourceMsg_BitstreamAdded message to Renderer. A failed request
+// triggers EncodedVideoSourceMsg_BitstreamRemoved message.
+IPC_MESSAGE_CONTROL3(EncodedVideoSourceHostMsg_AddBitstream,
+                     int /* device_id */,
+                     int /* stream_id */,

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

How does the renderer get these IDs?  (device & stream)

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Added comments on device_id & stream_id.

+                     media::VideoEncodingParameters /* params */)
+
+// Message from Renderer to Browser process to stop streaming a bitstream. When
+// Browser has finalized the bitstream it will trigger
+// EncodedVideoSourceMsg_BitstreamRemoved message back from Browser to Renderer.
+// Renderer must be prepared to receive EncodedVideoSourceMsg_BitstreamReady
+// messages until it has received the EncodedVideoSourceMsg_BitstreamRemoved
+// message.
+IPC_MESSAGE_CONTROL2(EncodedVideoSourceHostMsg_RemoveBitstream,

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

Now you have Add/Remove -> Added/Removed but Removed also encompasses Error.
I think these would be better as 
Create -> Created
Destroy -> Destroyed
then if Destroyed encompassed Error it wouldn't be as surprising. 

But I still think conflating Removed & Error is a bad move.  It'll make testing
harder, and it'll make it impossible to write well-behaved clients (which may be
happy to see a stream end but unhappy to see it error out).

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Changed wording for Add/Remove wording to Create/Destroy.
And also added status code for the bitstream destroyed message to differentiate
between different reasons why it might occur.

+                     int /* device_id */,
+                     int /* stream_id */)
+
+// Message from Renderer to Browser process to set a stream's bitstream config.
+// Will always result in EncodedVideoSourceMsg_BitstreamConfigChanged containing
+// currently active parameters, regardless of whether this call succeeded or
+// not.

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

This guarantee is not present in the .h.

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Added to the header.

+IPC_MESSAGE_CONTROL3(EncodedVideoSourceHostMsg_TrySetBitstreamConfig,
+                     int /* device_id */,
+                     int /* stream_id */,
+                     media::RuntimeVideoEncodingParameters /* params */)
+
+// Message from Renderer to Browser process to request special frames from the
+// encoded video source. There will be no acknowledgement for this request and
+// the effect is only visible in the bitstream buffers passed to client
+// through the EncodedVideoSourceMsg_BitstreamReady message. This request is
+// served on a best-effort basis and client is not given any guarantees of the
+// realization or timing of the request. Flags parameter will be interpreted in
+// format-specific manner using enumerations.

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

s/format/codec/

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Done.

+IPC_MESSAGE_CONTROL3(EncodedVideoSourceHostMsg_RequestSpecialFrame,
+                     int /* device_id */,
+                     int /* stream_id */,
+                     int /* flags */)
+
+// Message from Browser to Renderer process to acknowledge a request to add an
+// encoded video bitstream. Modifiable boolean value informs whether client can
+// adjust the bitstream config using TrySetBitstreamConfig. When this message
+// occurs bitstream can be considered to be streaming and Renderer should be
+// ready to start accepting EncodedVideoSourceMsg_BitstreamReady messages and
+// buffers contained within them.
+IPC_MESSAGE_CONTROL3(EncodedVideoSourceMsg_BitstreamAdded,
+                     int /* device id */,
+                     int /* stream id */,
+                     bool /* modifiable */)
+
+// Message from Browser to Renderer process to acknowledge a request to remove
+// an encoded video bitstream. Also used to signal renderer process that an
+// unrecoverable error has occurred and as a result bitstream has been removed.
+IPC_MESSAGE_CONTROL2(EncodedVideoSourceMsg_BitstreamRemoved,
+                     int /* device id */,
+                     int /* stream id */)
+
+// Message from Browser to Renderer process to inform the clients of the current
+// encoding parameters, regardless of whether the previous request to change
+// them has been successful or not. This is called usually as a response to
+// EncodedVideoSourceHostMsg_TrySetBitstreamConfig during runtime, but can occur
+// also as a result of config change initiated by encoder or other clients in
+// the system. E.g. if there are multiple clients and bitstream config change is
+// requested from one client, all clients should be prepared to handle the
+// configuration change.
+IPC_MESSAGE_CONTROL3(EncodedVideoSourceMsg_BitstreamConfigChanged,
+                     int /* device id */,
+                     int /* stream id */,
+                     media::RuntimeVideoEncodingParameters /* current_params */)
+
+// Message from Browser to Renderer process indicating that a bitstream buffer
+// is available for the stream.
+// TODO(vmr): Consider using SharedMemory instead.

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

I really think you want this now, not in the future.
Since shmem is internally refcounted anyway (by the OS) it doesn't make coding
for multiple clients any harder than it would otherwise be.

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Fair enough. Done.

+IPC_MESSAGE_CONTROL4(EncodedVideoSourceMsg_BitstreamReady,
+                     int /* device id */,
+                     int /* stream_id */,
+                     std::vector<uint8> /* bitstream */,
+                     media::BufferEncodingMetadata /* metadata */)
+