Add metadata to media::VideoFrame and plumb it through IPC/MediaStream.

Add metadata to media::VideoFrame and plumb it through IPC/MediaStream.

This change has three main goals: First, to be able to pass extra
information associated with each VideoFrame from the capture source to
the downstream consumers (see bugs for details).  Second, to eliminate
redundancies in the current MediaStreamVideoSink API; specifically,
media::VideoFrame contains most of the same properties as
media::VideoCaptureFormat.  Third, to fully support the separate
VideoFrame concepts of "coded size" versus "visible size" in the capture
pipeline, rather than force all producers/consumers to deal with packed
data.  (Using packed frame sizes can be suboptimal for performance in
some use cases.)

The metadata is stored in a base::DictionaryValue owned by
media::VideoFrame to allow for structured data passing.  DictionaryValue
is a great choice since an efficient IPC (de)serialization
implementation already exists, and the metadata can be easily
pretty-printed for logging where needed.  Also, it's logical for
VideoFrame to own the metadata, as both need to be passed/shared
together across threads without copying.

Finally, this change includes one new use of the metadata as a
motivation for its existence: The tab/desktop capture code now includes
capture timing information, which will allow Cast streaming sessions to
be analyzed for user experience improvements.  In the future, some of
the special-use-case data members in VideoFrame should be moved into
the metadata.

BUG=461116, 462101
Committed: https://crrev.com/78807dc7968c9a397326aef113435cbca486f96c
Cr-Commit-Position: refs/heads/master@{#318954}

[miu, 2015-02-26 04:53:07]

miu@chromium.org changed reviewers:
+ hclam@chromium.org

[miu, 2015-02-26 05:04:23]

Patchset #1 (id:1) has been deleted

[miu, 2015-02-26 05:09:51]

hclam: Please read the change description, then start by looking at
media/base/video_frame.h, media/base/video_capturer_source.h, and
media/cast/sender/video_sender.cc.  If those 3 files look good to you, then
please proceed with reviewing the rest.

[Alpha Left Google, 2015-02-26 21:17:23]

I like the idea about adding a dictionary to VideoFrame and use it to pass
timestamps.

However I'm not familiar with how VideoCaptureFormat is used in the capture
pipeline. perkj@ would know better than I do.

https://codereview.chromium.org/955253002/diff/40001/content/browser/media/ca...
File content/browser/media/capture/content_video_capture_device_core.cc (right):

https://codereview.chromium.org/955253002/diff/40001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:219:
"captureBeginTimeTicks",
Can these names be defined in video_frame.h as well?

[miu, 2015-02-26 23:25:18]

> I like the idea about adding a dictionary to VideoFrame and use it to pass
> timestamps.
> 
> However I'm not familiar with how VideoCaptureFormat is used in the capture
> pipeline. perkj@ would know better than I do.

I've checked all the browser- and renderer-side code.  It's simply not used,
except for a few places where the |frame_rate| value was needed.  The rest of
VideoCaptureFormat was redundant, as this information was already present in
VideoFrame (e.g., as coded_size, visible_rect, etc.).

Response to your review comment is below.  Does the rest of the change look good
to you too?

https://codereview.chromium.org/955253002/diff/40001/content/browser/media/ca...
File content/browser/media/capture/content_video_capture_device_core.cc (right):

https://codereview.chromium.org/955253002/diff/40001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:219:
"captureBeginTimeTicks",
On 2015/02/26 21:17:23, Alpha wrote:
> Can these names be defined in video_frame.h as well?

I'm uncomfortable with starting the practice of putting all the possible
metadata keys in video_frame.h.  The captureBegin/End keys are very specific to
one producer (tab/desktop capture) combined with one consumer (Cast streaming). 
The other issue is where we might centrally locate the definition of the keys,
but there is really no good place (i.e., the tab capture code cannot DEPS on
media/cast and vice-versa).

[miu, 2015-02-26 23:26:45]

miu@chromium.org changed reviewers:
+ perkj@chromium.org

[miu, 2015-02-26 23:26:46]

perkj: PTAL.

[Alpha Left Google, 2015-02-26 23:28:45]

On 2015/02/26 23:25:18, miu wrote:
> > I like the idea about adding a dictionary to VideoFrame and use it to pass
> > timestamps.
> > 
> > However I'm not familiar with how VideoCaptureFormat is used in the capture
> > pipeline. perkj@ would know better than I do.
> I'm uncomfortable with starting the practice of putting all the possible
> metadata keys in video_frame.h.  The captureBegin/End keys are very specific
to
> one producer (tab/desktop capture) combined with one consumer (Cast
streaming). 
> The other issue is where we might centrally locate the definition of the keys,
> but there is really no good place (i.e., the tab capture code cannot DEPS on
> media/cast and vice-versa).

If I have to choose between hardcoding a string in both capturer and cast
media::cast::VideoSender versus have a string defined somewhere. I would much
prefer to have it defined in a common header file. What about a separate header
file that defines these keys in media/base?

[miu, 2015-02-27 01:23:32]

On 2015/02/26 23:28:45, Alpha wrote:
> What about a separate header file that defines these keys in media/base?

Done.  PTAL.

[miu, 2015-02-27 01:24:07]

miu@chromium.org changed reviewers:
- perkj@chromium.org

[miu, 2015-02-27 01:24:07]

Remove perkj as reviewer due to leave.

[miu, 2015-02-27 01:58:37]

miu@chromium.org changed reviewers:
+ bbudge@chromium.org, dalecurtis@chromium.org, tommi@chromium.org

[miu, 2015-02-27 01:58:37]

Adding reviewers for OWNERS approval:

bbudge: content/renderer/pepper/*

dalecurtis: media/base/*

tommi: content/browser/renderer_host/media/*
    content/common/media/video_capture_messages.h
    content/renderer/media/*

[DaleCurtis, 2015-02-27 17:38:10]

Hmm, I have mixed thoughts on this. On one hand we get a request for new
metadata every week, so this approach seems much more sound than churning the
definition. On the other hand, do we really want to use a DictionaryValue like
this for metadata which might be required by some endpoints?

Since we can't control reviews of VideoFrame metadata after this change I think
it's highly likely we'll see "optional" metadata turn into required for some
endpoints.

[miu, 2015-02-27 19:54:28]

On 2015/02/27 17:38:10, DaleCurtis wrote:
> Hmm, I have mixed thoughts on this. On one hand we get a request for new
> metadata every week, so this approach seems much more sound than churning the
> definition. On the other hand, do we really want to use a DictionaryValue like
> this for metadata which might be required by some endpoints?

My thoughts on this were that non-optional metadata should either be separate
properties in VideoFrame (e.g., timestamp()), or passed as separate objects
alongside VideoFrame.  Only optional metadata should go in this dictionary.

Proposal: What if I made this more explicit by renaming the
VideoFrame::metadata() method to optional()?  Then, code would always look like:

  double foo;
  if (video_frame->optional().GetDouble("myProp", &foo)) {
    // do something with foo
  } else {
    // make do without the value
  }

This should make developers and code reviewers think twice about expecting
required values to be present.

Another proposal: I could also subclass DictionaryValue and only provide public
getter methods that are annotated with WARN_UNUSED_RESULT.  That way, developers
won't be allowed to compile code that assumes a value must be present.  Cool,
eh?

What do you think of these ideas?

> Since we can't control reviews of VideoFrame metadata after this change I
think
> it's highly likely we'll see "optional" metadata turn into required for some
> endpoints.

Alpha was right to make me create the media/base/video_frame_metadata_keys.*
files to centralize the keys.  This means, when keys must travel "long
distances," developers will need to define their keys in this file; and that
would require a media OWNER review.  When the keys are travelling "short
distances" (e.g., closely related code in the same file or dir), it's perfectly
reasonable to let localized OWNERS to manage their own metadata keys.

[Alpha Left Google, 2015-02-27 19:58:21]

On 2015/02/27 19:54:28, miu wrote:
> On 2015/02/27 17:38:10, DaleCurtis wrote:
> > Hmm, I have mixed thoughts on this. On one hand we get a request for new
> > metadata every week, so this approach seems much more sound than churning
the
> > definition. On the other hand, do we really want to use a DictionaryValue
like
> > this for metadata which might be required by some endpoints?
> 
> My thoughts on this were that non-optional metadata should either be separate
> properties in VideoFrame (e.g., timestamp()), or passed as separate objects
> alongside VideoFrame.  Only optional metadata should go in this dictionary.
> 
> Proposal: What if I made this more explicit by renaming the
> VideoFrame::metadata() method to optional()?  Then, code would always look
like:
> 
>   double foo;
>   if (video_frame->optional().GetDouble("myProp", &foo)) {
>     // do something with foo
>   } else {
>     // make do without the value
>   }
> 
> This should make developers and code reviewers think twice about expecting
> required values to be present.
> 
> Another proposal: I could also subclass DictionaryValue and only provide
public
> getter methods that are annotated with WARN_UNUSED_RESULT.  That way,
developers
> won't be allowed to compile code that assumes a value must be present.  Cool,
> eh?
> 
> What do you think of these ideas?
> 
> > Since we can't control reviews of VideoFrame metadata after this change I
> think
> > it's highly likely we'll see "optional" metadata turn into required for some
> > endpoints.
> 
> Alpha was right to make me create the media/base/video_frame_metadata_keys.*
> files to centralize the keys.  This means, when keys must travel "long
> distances," developers will need to define their keys in this file; and that
> would require a media OWNER review.  When the keys are travelling "short
> distances" (e.g., closely related code in the same file or dir), it's
perfectly
> reasonable to let localized OWNERS to manage their own metadata keys.

I think Dale's concern about misuse of the Dictionary in the future is
justified.
Particularly it is easy to not follow reviews and hardcode a string here and
there.

One way to solve this is to replace the strings with a enum. Adding an enum will
have to go through a formal review.

[DaleCurtis, 2015-02-27 20:18:18]

I think my concerns would be obviated if the only keys allowed were those in the
metdata keys file; that way we can maintainer OWNERS approval. However you want
to make that work is fine with me.

[miu, 2015-02-27 21:10:01]

On 2015/02/27 20:18:18, DaleCurtis wrote:
> I think my concerns would be obviated if the only keys allowed were those in
the
> metdata keys file; that way we can maintainer OWNERS approval. However you
want
> to make that work is fine with me.

Okay.  Sounds like everyone's much more concerned about abuse than I am.  I'll
force key definition via an enum.  I'll ping once I've uploaded a new patch set.

[miu, 2015-02-27 23:37:41]

On 2015/02/27 21:10:01, miu wrote:
> On 2015/02/27 20:18:18, DaleCurtis wrote:
> > I think my concerns would be obviated if the only keys allowed were those in
> the
> > metdata keys file; that way we can maintainer OWNERS approval. However you
> want
> > to make that work is fine with me.

Dale and Alpha: PTAL at patch set 5.  I wrote a small wrapper around
base::DictionaryValue so that the keys have to be in a centralized enum (in
media/base/video_frame_metadata.h).

[mcasas, 2015-02-28 00:56:25]

mcasas@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2015-02-28 00:56:26]

+1 on streamlining all that zoo of metadata associated to
VideoFrame.

Feels a bit silly that we need to recompose several types of
metadata after crossing IPC to Renderlands, e.g. 
OnMailboxBufferReceived() has |packed_frame_size|,
|timestamp| and, now, |metadata|. But I understand
there's a TODO in VideoFrame to extend |metadata_|,
am I right? I see Timestamp at the moment is a best-effort
number, so it could perfectly go in there, or is there any
real use of that number? (|allow_overlay_|????)

Since I have my class-member-chopping-hatchet at hand, I 
could envision a future where two of these three: coded_size, 
natural_size, visible_size are optional fields in the metadata,
since these are the same for all memory-backed capture 
devices (not for Desktop/Tab) then again VideoFrame is used 
by others so it'd need going case-by-case.

[DaleCurtis, 2015-03-02 23:23:28]

Otherwise this lgtm.

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame....
media/base/video_frame.h:299: VideoFrameMetadata& metadata() { return metadata_;
}
We don't typically return non-const refs. Use a pointer?

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame_...
File media/base/video_frame_metadata.cc (right):

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame_...
media/base/video_frame_metadata.cc:15: return
std::string(reinterpret_cast<char*>(&key), sizeof(key));
Hmm, this creates an std::string() with 0x00 and 0x01, etc bytes in it; which
seems fine, but given that base::DictionaryValue keys have special characters
(".") may be risky. It also means the dictionary can't easily be dumped to
string.

[Alpha Left Google, 2015-03-02 23:26:03]

On 2015/03/02 23:23:28, DaleCurtis wrote:
> Otherwise this lgtm.
> 
> https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame.h
> File media/base/video_frame.h (right):
> 
>
https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame....
> media/base/video_frame.h:299: VideoFrameMetadata& metadata() { return
metadata_;
> }
> We don't typically return non-const refs. Use a pointer?
> 
>
https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame_...
> File media/base/video_frame_metadata.cc (right):
> 
>
https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame_...
> media/base/video_frame_metadata.cc:15: return
> std::string(reinterpret_cast<char*>(&key), sizeof(key));
> Hmm, this creates an std::string() with 0x00 and 0x01, etc bytes in it; which
> seems fine, but given that base::DictionaryValue keys have special characters
> (".") may be risky. It also means the dictionary can't easily be dumped to
> string.

LGTM on cast related files.

[miu, 2015-03-03 04:31:05]

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame....
media/base/video_frame.h:299: VideoFrameMetadata& metadata() { return metadata_;
}
On 2015/03/02 23:23:28, DaleCurtis wrote:
> We don't typically return non-const refs. Use a pointer?

Oh, that's right.  Done.

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame_...
File media/base/video_frame_metadata.cc (right):

https://codereview.chromium.org/955253002/diff/100001/media/base/video_frame_...
media/base/video_frame_metadata.cc:15: return
std::string(reinterpret_cast<char*>(&key), sizeof(key));
On 2015/03/02 23:23:28, DaleCurtis wrote:
> Hmm, this creates an std::string() with 0x00 and 0x01, etc bytes in it; which
> seems fine, but given that base::DictionaryValue keys have special characters
> (".") may be risky. It also means the dictionary can't easily be dumped to
> string.

I accounted for that by using the Get/SetXXXWithoutPathExpansion() family of
getters and setters which don't apply the special "." semantics.

I thought about this a bit more, and I don't think it would be much of a
performance overhead to use ASCII decimal digits for keys.  Actually, it turns
out to be more space-efficient (1 byte instead of 4).  So, I've made that
change.

[miu, 2015-03-03 04:31:21]

miu@chromium.org changed reviewers:
+ nasko@chromium.org

[miu, 2015-03-03 04:31:22]

nasko: PTAL at content/common/media/video_capture_messages.h for security OWNERS
approval.

[bbudge, 2015-03-03 04:43:18]

pepper lgtm

[tommi (sloooow) - chröme, 2015-03-03 10:06:16]

lgtm for */media/*

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:180: &frame_rate))
nit: {}

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:43: void
DoRenderFrameOnIOThread(scoped_refptr<media::VideoFrame> video_frame);
nit: const & ?

[nasko, 2015-03-03 18:30:02]

I will not be able to get to this review within the next 24 hours. If you need
quicker turnaround, feel free to pick someone else from the set of IPC
reviewers.

[miu, 2015-03-03 19:52:30]

miu@chromium.org changed reviewers:
+ tsepez@chromium.org
- nasko@chromium.org

[miu, 2015-03-03 19:52:31]

tsepez: Would you PTAL at content/common/media/video_messages.h for security
OWNERS approval?

[Tom Sepez, 2015-03-03 20:06:20]

It's sometimes hard to review the security impact of items passed inside a
dictionary; although we can be confident that the serialization is sane, we
can't know whether things like origins, URLs, security indicators, capabilities
etc. are being tunneled through the dictionary.

In your case, it sounds like this isn't going to be the case, but just to
confirm, can you enumerate the kinds of data you intend on passing?  You could
save me some time digging through your code if you could do this.  Thanks.

[miu, 2015-03-03 20:31:01]

On 2015/03/03 20:06:20, Tom Sepez wrote:
> It's sometimes hard to review the security impact of items passed inside a
> dictionary; although we can be confident that the serialization is sane, we
> can't know whether things like origins, URLs, security indicators,
capabilities
> etc. are being tunneled through the dictionary.
> 
> In your case, it sounds like this isn't going to be the case, but just to
> confirm, can you enumerate the kinds of data you intend on passing?  You could
> save me some time digging through your code if you could do this.  Thanks.

We will be passing metrics related to video capture only, and definitely not
URLs or user-sensitive data.  Things like fixed and current estimated frame
rates, color profile data for the video frame, camera sensor brightness levels,
and miscellaneous timestamps for performance/quality.  In this change, I am only
adding fixed frame rate and capture begin/end timestamps. 

The dictionary is actually very restrictive, as it is only accessed via
media::VideoFrame::metadata(), which is a media::VideoFrameMetadata instance
that requires all keys be registered in a centralized enum (see
media/base/video_frame_metadata.* files).

The VideoFrameMetadata contents are copied to a base::DictionaryValue and back
ONLY for IPC messaging (in
content/browser/renderer_host/media/video_capture_controller.cc and
content/renderer/media/video_capture_impl.cc).

[Tom Sepez, 2015-03-03 20:40:14]

On 2015/03/03 20:31:01, miu wrote:
> On 2015/03/03 20:06:20, Tom Sepez wrote:
> > It's sometimes hard to review the security impact of items passed inside a
> > dictionary; although we can be confident that the serialization is sane, we
> > can't know whether things like origins, URLs, security indicators,
> capabilities
> > etc. are being tunneled through the dictionary.
> > 
> > In your case, it sounds like this isn't going to be the case, but just to
> > confirm, can you enumerate the kinds of data you intend on passing?  You
could
> > save me some time digging through your code if you could do this.  Thanks.
> 
> We will be passing metrics related to video capture only, and definitely not
> URLs or user-sensitive data.  Things like fixed and current estimated frame
> rates, color profile data for the video frame, camera sensor brightness
levels,
> and miscellaneous timestamps for performance/quality.  In this change, I am
only
> adding fixed frame rate and capture begin/end timestamps. 
> 
> The dictionary is actually very restrictive, as it is only accessed via
> media::VideoFrame::metadata(), which is a media::VideoFrameMetadata instance
> that requires all keys be registered in a centralized enum (see
> media/base/video_frame_metadata.* files).
> 
> The VideoFrameMetadata contents are copied to a base::DictionaryValue and back
> ONLY for IPC messaging (in
> content/browser/renderer_host/media/video_capture_controller.cc and
> content/renderer/media/video_capture_impl.cc).
OK. LGTM

[miu, 2015-03-03 21:31:52]

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:180: &frame_rate))
On 2015/03/03 10:06:15, tommi wrote:
> nit: {}

Done.

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/955253002/diff/160001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:43: void
DoRenderFrameOnIOThread(scoped_refptr<media::VideoFrame> video_frame);
On 2015/03/03 10:06:15, tommi wrote:
> nit: const & ?

Done.

[miu, 2015-03-03 21:32:00]

The CQ bit was checked by miu@chromium.org

[miu, 2015-03-03 21:32:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, hclam@chromium.org, bbudge@chromium.org,
tommi@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/955253002/#ps180001
(title: "tommi's nits addressed")

[commit-bot: I haz the power, 2015-03-03 21:32:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/955253002/180001

[commit-bot: I haz the power, 2015-03-03 23:08:16]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-03-03 23:09:22]

Patchset 9 (id:??) landed as
https://crrev.com/78807dc7968c9a397326aef113435cbca486f96c
Cr-Commit-Position: refs/heads/master@{#318954}