Passing frame decoded callback to the codec

Cast: Passing the frame decoded callback to the codec.

This cl cleans up the video receiver from handling the raw
frames, and will align the frame handling of the cast vp8
decoder and the chromium/ffmpeg h264 decoder (to be added).

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

[mikhal, 2013-11-06 00:09:48]

Please review, 
-Mikhal

[Alpha Left Google, 2013-11-06 01:58:40]

On 2013/11/06 00:09:48, mikhal wrote:
> Please review, 
> -Mikhal

Please fix the CL description to the following format:

<Title 72 chars max>
<Empty Line>
<A paragraph of description 72 chars max per line. A couple full sentences will
do.>

This is a comment from other committers as well. It will make the git log
message much nicer and people who doesn't know the code have a brief
understanding of what's going on.

[Alpha Left Google, 2013-11-06 02:22:06]

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
File media/cast/test/encode_decode_test.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:22: static const int64 kStartMillisecond =
GG_INT64_C(1245);
The other const values don't have static, why this one needs it?

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:30: class EncodeDecodeTestFrameCallback :
nit: put this in an anonymous namespace.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:31: public
base::RefCountedThreadSafe<EncodeDecodeTestFrameCallback> {
nit: I think this is 4 space indentation.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:35: original_frame_() {
nit: This line is not needed.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:55: int num_called() {
nit: int num_called() const {}

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:67: 
nit: remove this empty line.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:97: virtual void SetUp() {
nit: while you're at it. This should SetUp() OVERRIDE.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:107: virtual void TearDown() {
nit: while you're at it. This should TearDown() OVERRIDE.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:129: base::TimeTicks render_time;
nit: There's no need to have this local variable.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:130: VideoFrameDecodedCallback
frame_decoded_callback =
nit: We don't usually define the callback as a local variable unless it is
necessary. For this case you can just move this to Decode().

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:133: decoder_->Decode(&encoded_frame,
render_time, frame_decoded_callback);
You can replace render_time with base::TimeTicks(). Also define the callback
inline.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.cc:38: VLOG(1) << "VP8 decode
frame:" << static_cast<int>(encoded_frame->frame_id)
There's 3 static_cast<int> for the frame id. I'd define a const local variable
for it.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.h (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:25:
scoped_refptr<CastEnvironment> cast_environment);
The usual style for scoped_refptr is to pass it as const ref, i.e. const
scoped_refptr<T>&

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:28: // Decode encoded image
(as a part of a video stream). The decoded frame will
nit: Decode a frame (...).

"Decode encoded image" seems a bit redundant.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:32: bool Decode(const
EncodedVideoFrame* encoded_frame,
Is the ownership of |encoded_frame| passed? If not please make a comment. If the
ownership is transferred please make it a scoped_ptr<T>.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:35: frame_decoded_callback);
You can pass the callback by value instead of const ref.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_decoder.h (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder.h:30: const VideoFrameDecodedCallback&
Call back doesn't need to be const ref. You can pass it by value.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_decoder_unittest.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:24: static const int64
kStartMillisecond = GG_INT64_C(1245);
nit: This doesn't need to be static right?

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:26: class
DecodeTestFrameCallback :
Put this in an anonymous namespace otherwise it might clash with callback
defined in other source file.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:45: test_callback_(new
DecodeTestFrameCallback()) {
nit: indentation is incorrect.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:73: frame_decoded_callback),
"Empty video frame");
nit: indentation is incorrect, this should align with &encoded_frame.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:86: frame_decoded_callback),
"Invalid codec");
nit: indentation is incorrect, it should align with &encoded_frame.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_receiver.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:183: bool success  =
video_decoder_->DecodeVideoFrame(encoded_frame.get(),
nit: I'd drop this local variable. You can call the method directly within the
if statement.

[mikhal, 2013-11-06 18:29:16]

PTAL

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
File media/cast/test/encode_decode_test.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:22: static const int64 kStartMillisecond =
GG_INT64_C(1245);
The are all local to this file - making them all static (aligning with the rest
of the Cast code).
On 2013/11/06 02:22:06, Alpha wrote:
> The other const values don't have static, why this one needs it?

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:30: class EncodeDecodeTestFrameCallback :
On 2013/11/06 02:22:06, Alpha wrote:
> nit: put this in an anonymous namespace. 

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:31: public
base::RefCountedThreadSafe<EncodeDecodeTestFrameCallback> {
On 2013/11/06 02:22:06, Alpha wrote:
> nit: I think this is 4 space indentation.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:35: original_frame_() {
On 2013/11/06 02:22:06, Alpha wrote:
> nit: This line is not needed.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:55: int num_called() {
On 2013/11/06 02:22:06, Alpha wrote:
> nit: int num_called() const {}
> 

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:67: 
On 2013/11/06 02:22:06, Alpha wrote:
> nit: remove this empty line.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:97: virtual void SetUp() {
On 2013/11/06 02:22:06, Alpha wrote:
> nit: while you're at it. This should SetUp() OVERRIDE.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:107: virtual void TearDown() {
On 2013/11/06 02:22:06, Alpha wrote:
> nit: while you're at it. This should TearDown() OVERRIDE.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:129: base::TimeTicks render_time;
On 2013/11/06 02:22:06, Alpha wrote:
> nit: There's no need to have this local variable.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:130: VideoFrameDecodedCallback
frame_decoded_callback =
On 2013/11/06 02:22:06, Alpha wrote:
> nit: We don't usually define the callback as a local variable unless it is
> necessary. For this case you can just move this to Decode().

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/test/encode_de...
media/cast/test/encode_decode_test.cc:133: decoder_->Decode(&encoded_frame,
render_time, frame_decoded_callback);
On 2013/11/06 02:22:06, Alpha wrote:
> You can replace render_time with base::TimeTicks(). Also define the callback
> inline.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.cc:38: VLOG(1) << "VP8 decode
frame:" << static_cast<int>(encoded_frame->frame_id)
On 2013/11/06 02:22:06, Alpha wrote:
> There's 3 static_cast<int> for the frame id. I'd define a const local variable
> for it.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.h (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:25:
scoped_refptr<CastEnvironment> cast_environment);
This way is consistent with the rest of Cast. Will make such a change throughout
the code in a designated cl. 
On 2013/11/06 02:22:06, Alpha wrote:
> The usual style for scoped_refptr is to pass it as const ref, i.e. const
> scoped_refptr<T>&

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:28: // Decode encoded image
(as a part of a video stream). The decoded frame will
On 2013/11/06 02:22:06, Alpha wrote:
> nit: Decode a frame (...).
> 
> "Decode encoded image" seems a bit redundant.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:32: bool Decode(const
EncodedVideoFrame* encoded_frame,
On 2013/11/06 02:22:06, Alpha wrote:
> Is the ownership of |encoded_frame| passed? If not please make a comment. If
the
> ownership is transferred please make it a scoped_ptr<T>.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:35: frame_decoded_callback);
On 2013/11/06 02:22:06, Alpha wrote:
> You can pass the callback by value instead of const ref.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_decoder.h (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder.h:30: const VideoFrameDecodedCallback&
On 2013/11/06 02:22:06, Alpha wrote:
> Call back doesn't need to be const ref. You can pass it by value.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_decoder_unittest.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:24: static const int64
kStartMillisecond = GG_INT64_C(1245);
Keeping for consistency - Static through out the cast code.
On 2013/11/06 02:22:06, Alpha wrote:
> nit: This doesn't need to be static right?

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:26: class
DecodeTestFrameCallback :
On 2013/11/06 02:22:06, Alpha wrote:
> Put this in an anonymous namespace otherwise it might clash with callback
> defined in other source file.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:45: test_callback_(new
DecodeTestFrameCallback()) {
On 2013/11/06 02:22:06, Alpha wrote:
> nit: indentation is incorrect.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_decoder_unittest.cc:86: frame_decoded_callback),
"Invalid codec");
On 2013/11/06 02:22:06, Alpha wrote:
> nit: indentation is incorrect, it should align with &encoded_frame.

Done.

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_receiver.cc (right):

https://codereview.chromium.org/59753007/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:183: bool success  =
video_decoder_->DecodeVideoFrame(encoded_frame.get(),
On 2013/11/06 02:22:06, Alpha wrote:
> nit: I'd drop this local variable. You can call the method directly within the
> if statement.

Done.

[Alpha Left Google, 2013-11-06 18:41:48]

LGTM after the following changes.

You were right before about const SomeCallback&. I just talked to the author of
callback.h to confirm. Sorry for misleading you!

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.cc (right):

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/codecs/vp8/vp8_decoder.cc:36: const
VideoFrameDecodedCallback frame_decoded_cb) {
I'm sorry but this should be a const ref, you were right before.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/codecs/vp8/vp8_decoder.cc:37: int frame_id_int =
static_cast<int>(encoded_frame->frame_id);
nit: const int frame_id_int.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.h (right):

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:34: const
VideoFrameDecodedCallback frame_decoded_cb);
I'm so sorry! I just talked to the author of callback.h, it should be const ref
after all. Sorry!

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

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/video_decoder.h:30: const VideoFrameDecodedCallback
frame_decoded_cb);
Sorry again this should be const ref, you were right before.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
File media/cast/video_receiver/video_decoder_unittest.cc (right):

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/video_decoder_unittest.cc:72:
decoder_->DecodeVideoFrame(&encoded_frame, render_time, base::Bind(
nit: Indentation should be like this:

EXPECT_DEATH(
    decoder_->DecodeVideoFrame(
        &encoded_frame, render_time, base::Bind(frame_decoded_callback),
    "Empty video frame");

"&DecodeTestFrameCallback::DecodeComplete" shouldn't be align to "decoder_->",
it should be indented by 4 extra spaces, but then I would move base::Bind() to
next line.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/video_decoder_unittest.cc:85:
&DecodeTestFrameCallback::DecodeComplete, test_callback_)),
nit: Same here indentation is incorrect.

[mikhal, 2013-11-06 18:53:07]

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.cc (right):

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/codecs/vp8/vp8_decoder.cc:36: const
VideoFrameDecodedCallback frame_decoded_cb) {
On 2013/11/06 18:41:49, Alpha wrote:
> I'm sorry but this should be a const ref, you were right before.

Done.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/codecs/vp8/vp8_decoder.cc:37: int frame_id_int =
static_cast<int>(encoded_frame->frame_id);
On 2013/11/06 18:41:49, Alpha wrote:
> nit: const int frame_id_int.

Done.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
File media/cast/video_receiver/codecs/vp8/vp8_decoder.h (right):

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/codecs/vp8/vp8_decoder.h:34: const
VideoFrameDecodedCallback frame_decoded_cb);
np, will revert. Thanks for checking!
On 2013/11/06 18:41:49, Alpha wrote:
> I'm so sorry! I just talked to the author of callback.h, it should be const
ref
> after all. Sorry!

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

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/video_decoder.h:30: const VideoFrameDecodedCallback
frame_decoded_cb);
On 2013/11/06 18:41:49, Alpha wrote:
> Sorry again this should be const ref, you were right before.

Done.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
File media/cast/video_receiver/video_decoder_unittest.cc (right):

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/video_decoder_unittest.cc:72:
decoder_->DecodeVideoFrame(&encoded_frame, render_time, base::Bind(
On 2013/11/06 18:41:49, Alpha wrote:
> nit: Indentation should be like this:
> 
> EXPECT_DEATH(
>     decoder_->DecodeVideoFrame(
>         &encoded_frame, render_time, base::Bind(frame_decoded_callback),
>     "Empty video frame");
> 
> "&DecodeTestFrameCallback::DecodeComplete" shouldn't be align to "decoder_->",
> it should be indented by 4 extra spaces, but then I would move base::Bind() to
> next line.

Done.

https://codereview.chromium.org/59753007/diff/350001/media/cast/video_receive...
media/cast/video_receiver/video_decoder_unittest.cc:85:
&DecodeTestFrameCallback::DecodeComplete, test_callback_)),
On 2013/11/06 18:41:49, Alpha wrote:
> nit: Same here indentation is incorrect.

Done.

[commit-bot: I haz the power, 2013-11-06 19:00:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@google.com/59753007/440001

[commit-bot: I haz the power, 2013-11-06 22:20:08]

Change committed as 233396