[Cast] EncodedAudioFrame+EncodedVideoFrame+reference_time --> EncodedFrame

[Cast] EncodedAudioFrame+EncodedVideoFrame+reference_time --> EncodedFrame

Replace the use of struct EncodedAudioFrame and struct EncodedVideoFrame
with a unified struct EncodedFrame throughout src/media/cast.  This
allows for later merging of duplicated code throughout the stack.

The new struct drops the unused codec data member and adds a new data
member, reference_time.  codec is unnecessary because this is known in
all contexts where an EncodedFrame is produced/consumed.  The new
reference_time data member is added because virtually all interfaces
that passed around an EncodedXXXFrame were also passing the frame's
reference_time alongside.

Testing: Updated and ran all cast_unittests, plus manual testing
between multiple Cast sender and receiver implementations.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271594

[miu, 2014-05-15 02:46:09]

hclam: PTAL.  This is all pretty simple stuff here.  Just take a look at
cast_transport_config.h first, and then the rest is just the refactoring around
that.

[Alpha Left Google, 2014-05-15 22:11:22]

This is a very big patch. This is the first pass and I'm calling out the
important things first.

https://codereview.chromium.org/288103002/diff/1/media/cast/framer/frame_buff...
File media/cast/framer/frame_buffer.h (right):

https://codereview.chromium.org/288103002/diff/1/media/cast/framer/frame_buff...
media/cast/framer/frame_buffer.h:34: bool is_key_frame() const { return
frame_id_ == last_referenced_frame_id_; }
I don't think we need to change this.

A key frame has two meanings:
1. It can be decoded independently.
2. Subsequent frames will not depend on frames prior to the key frame. Meaning a
restart of the encoded stream.

While referenced frame id == frame id means it is independently decodable but it
doesn't necessarily mean a key frame.

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_config.h:95: uint32 base_frame_id;
I actually prefer the name before, i.e. referenced_frame_id. Calling this
|base_something| doesn't describe it more accurately.

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_config.h:104: // Reference time used for
synchronization between streams.  In the sender
In the sender the time is not used for synchronization. It is only used for
ordering between different streams in the pacer.

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_config.h:110: base::TimeTicks
reference_time;
Calling it reference_time doesn't seem to fit either of the purpose in sender
and receiver. Wouldn't |timestamp| be enough?

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc (right):

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc:89: const bool
is_key = (frame.frame_id == frame.base_frame_id);
As described before frame_id == base_frame_id doesn't necessarily make a key
frame.

[miu, 2014-05-16 04:27:25]

hclam: Addressed you first round comments.

hubbe: Could you take a look at the changes to cast_transport_config.h?  It
would be good to get a second set of eyes on that; especially since you might
have thoughts on the new Relationship enum for your work on VP8 3-buffer mode.

https://codereview.chromium.org/288103002/diff/1/media/cast/framer/frame_buff...
File media/cast/framer/frame_buffer.h (right):

https://codereview.chromium.org/288103002/diff/1/media/cast/framer/frame_buff...
media/cast/framer/frame_buffer.h:34: bool is_key_frame() const { return
frame_id_ == last_referenced_frame_id_; }
On 2014/05/15 22:11:23, Alpha wrote:
> I don't think we need to change this.
> 
> A key frame has two meanings:
> 1. It can be decoded independently.
> 2. Subsequent frames will not depend on frames prior to the key frame. Meaning
a
> restart of the encoded stream.
> 
> While referenced frame id == frame id means it is independently decodable but
it
> doesn't necessarily mean a key frame.

Done.

You make an excellent point.  I talked with hubbe@ earlier today and also
thought about this a bit more.  So, I've decided to add back the "is key frame"
in EncodedFrame (in the form of an enum).

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_config.h:95: uint32 base_frame_id;
On 2014/05/15 22:11:23, Alpha wrote:
> I actually prefer the name before, i.e. referenced_frame_id. Calling this
> |base_something| doesn't describe it more accurately.

Done.  After looking around more in the codec APIs, it looks like "reference
frame" is best for naming consistency.

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_config.h:104: // Reference time used for
synchronization between streams.  In the sender
On 2014/05/15 22:11:23, Alpha wrote:
> In the sender the time is not used for synchronization. It is only used for
> ordering between different streams in the pacer.

I've clarified the comment (that the sender generates this value for the
purposes of lip sync in the receiver).

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_config.h:110: base::TimeTicks
reference_time;
On 2014/05/15 22:11:23, Alpha wrote:
> Calling it reference_time doesn't seem to fit either of the purpose in sender
> and receiver. Wouldn't |timestamp| be enough? 

This is exactly the terminology used in Chapter 7 in the RTP book (see the
timing diagram on page 212).  I've reworked the comment a bit to clarify things,
but I think the naming of this field makes sense.

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc (right):

https://codereview.chromium.org/288103002/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc:89: const bool
is_key = (frame.frame_id == frame.base_frame_id);
On 2014/05/15 22:11:23, Alpha wrote:
> As described before frame_id == base_frame_id doesn't necessarily make a key
> frame.

Done.

[hubbe, 2014-05-16 17:13:19]

Mostly good, a few minor issues.
Since there is some naming involved, let's make sure Alpha likes the names too.

https://codereview.chromium.org/288103002/diff/10001/media/cast/test/end2end_...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:322:
reinterpret_cast<int16*>(audio_frame->mutable_bytes());
Why not ->bytes() ?

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:104: } relationship;
I think it's more clear to separate the enum declaration and the variable.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:104: } relationship;
I'm not sure I think "relationship" describes this variable very well.
Maybe "prerequisites"? "decode_requirements"?

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_receive...
File media/cast/video_receiver/video_decoder.h (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_receive...
media/cast/video_receiver/video_decoder.h:46: // TODO(miu): FIXME: This
assumption will break VP8 3-buffer mode.
?

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
File media/cast/video_sender/external_video_encoder.cc (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:231: if (!key_frame)
I think

if (key_frame)
  encoded_frame->referenced_frame_id = encoded_frame->frame_id;
else
  encoded_frame->referenced_frame_id = encoded_frame->frame_id - 1;

Would actually be clearer.

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
File media/cast/video_sender/fake_software_video_encoder.cc (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
media/cast/video_sender/fake_software_video_encoder.cc:36:
encoded_image->referenced_frame_id = frame_id_to_reference_;
This makes this codec behave like 3-buffer mode.
We probably want it to behave like !3-buffer mode, in which case this should be
encoded_image->framd_id -1.

[Alpha Left Google, 2014-05-16 18:35:21]

Looking good. Some nits and clarification of comments.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:89: enum Relationship {
It seems to me this enum should be called "Dependency".

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:91: UNKNOWN_RELATIONSHIP,
This is not used and will not be used. Please remove it.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:93: // Not decodable without the
reference frame.
This comment needs to be clarified: without the reference frame indicated by
|referenced_frame_id|.

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
File media/cast/video_sender/codecs/vp8/vp8_encoder.cc (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
media/cast/video_sender/codecs/vp8/vp8_encoder.cc:211: is_key_frame |=
!!(pkt->data.frame.flags & VPX_FRAME_IS_KEY);
There is only one packet of kind VPX_CODEC_CX_FRAME_PKT so this loop is
confusing.

I suggest you just assign is_key_frame instead of doing a OR operation and then
break afterwards.

[miu, 2014-05-16 21:28:46]

hclam and hubbe: Ready for another look.  Thanks.

https://codereview.chromium.org/288103002/diff/10001/media/cast/test/end2end_...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:322:
reinterpret_cast<int16*>(audio_frame->mutable_bytes());
On 2014/05/16 17:13:20, hubbe wrote:
> Why not ->bytes() ?

Is your question:

1. Why did I choose to call mutable_bytes() here instead of bytes()?
A: Because the int16's are being byte swapped on line 324, and stored back into
the array.

or

2. Why didn't I name the non-const accessor method bytes() in EncodedFrame?
A: Readability, mostly.  A habit from using proto bufs, I suppose.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:89: enum Relationship {
On 2014/05/16 18:35:22, Alpha wrote:
> It seems to me this enum should be called "Dependency".

Done.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:91: UNKNOWN_RELATIONSHIP,
On 2014/05/16 18:35:22, Alpha wrote:
> This is not used and will not be used. Please remove it.

Done.  It was being used to check that the field was actually getting set (while
I was testing this change).  I felt it was a good thing to leave in, but since
you disagree, I have no problem taking it back out.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:93: // Not decodable without the
reference frame.
On 2014/05/16 18:35:22, Alpha wrote:
> This comment needs to be clarified: without the reference frame indicated by
> |referenced_frame_id|.

Done.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:104: } relationship;
On 2014/05/16 17:13:20, hubbe wrote:
> I think it's more clear to separate the enum declaration and the variable.

Done.

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:104: } relationship;
On 2014/05/16 17:13:20, hubbe wrote:
> I'm not sure I think "relationship" describes this variable very well.
> Maybe "prerequisites"? "decode_requirements"?

Done.  Changed it to "dependency," per team discussion.

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_receive...
File media/cast/video_receiver/video_decoder.h (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_receive...
media/cast/video_receiver/video_decoder.h:46: // TODO(miu): FIXME: This
assumption will break VP8 3-buffer mode.
On 2014/05/16 17:13:20, hubbe wrote:
> ?

Done.  Oops, nothing to see here...

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
File media/cast/video_sender/codecs/vp8/vp8_encoder.cc (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
media/cast/video_sender/codecs/vp8/vp8_encoder.cc:211: is_key_frame |=
!!(pkt->data.frame.flags & VPX_FRAME_IS_KEY);
On 2014/05/16 18:35:22, Alpha wrote:
> There is only one packet of kind VPX_CODEC_CX_FRAME_PKT so this loop is
> confusing.
> 
> I suggest you just assign is_key_frame instead of doing a OR operation and
then
> break afterwards.

Done.

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
File media/cast/video_sender/external_video_encoder.cc (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:231: if (!key_frame)
On 2014/05/16 17:13:20, hubbe wrote:
> I think
> 
> if (key_frame)
>   encoded_frame->referenced_frame_id = encoded_frame->frame_id;
> else
>   encoded_frame->referenced_frame_id = encoded_frame->frame_id - 1;
> 
> Would actually be clearer.
> 
>    

Done.

[Alpha Left Google, 2014-05-17 01:00:37]

LGTM.

[hubbe, 2014-05-19 17:54:42]

LGTM

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/288103002/diff/10001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:91: UNKNOWN_RELATIONSHIP,
On 2014/05/16 21:28:46, miu wrote:
> On 2014/05/16 18:35:22, Alpha wrote:
> > This is not used and will not be used. Please remove it.
> 
> Done.  It was being used to check that the field was actually getting set
(while
> I was testing this change).  I felt it was a good thing to leave in, but since
> you disagree, I have no problem taking it back out.

I kind of liked UNKNOWN_RELATIONSHIP.

[miu, 2014-05-19 19:28:50]

jorgelo: PTAL at chrome/common/cast_messages.h.

[Jorge Lucangeli Obes, 2014-05-19 23:13:34]

cast_messages.h LGTM

[miu, 2014-05-20 01:51:10]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-05-20 01:51:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/288103002/110001

[commit-bot: I haz the power, 2014-05-20 04:45:36]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-20 06:40:52]

Change committed as 271594