Chromecast: adds test frame-segmenter code to support CMA unit tests.

Chromecast: adds test frame-segmenter code to support CMA unit tests.

R=xhwang@chromium.org,damienv@chromium.org
BUG=408189
Committed: https://crrev.com/794810208e249261735b14e682b763ce3aa2d614
Cr-Commit-Position: refs/heads/master@{#298090}

[gunsch, 2014-10-02 22:24:50]

gunsch@chromium.org changed reviewers:
+ damienv@chromium.org, xhwang@chromium.org

[gunsch, 2014-10-02 22:24:51]

PTAL, thanks!

[damienv1, 2014-10-03 00:06:23]

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
File chromecast/media/cma/test/DEPS (right):

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
chromecast/media/cma/test/DEPS:4: "+third_party/ffmpeg",
Only direct dependencies should be needed, right ?

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:280:
base::MessageLoop::current()->QuitWhenIdle();
I don't think that's the right location to call QuitWhenIdle (should not be done
in this file).
You should check with Sergey as to why it was done here (don't recall the
reason), but that does not belong logically to this file.

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
File chromecast/media/cma/test/frame_segmenter_for_test.h (right):

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.h:48: 
nit: remove last line.

[servolk, 2014-10-03 00:24:57]

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

[servolk, 2014-10-03 00:24:58]

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:280:
base::MessageLoop::current()->QuitWhenIdle();
On 2014/10/03 00:06:22, damienv1 wrote:
> I don't think that's the right location to call QuitWhenIdle (should not be
done
> in this file).
> You should check with Sergey as to why it was done here (don't recall the
> reason), but that does not belong logically to this file.

Well, the way it works at the moment is: in FFmpegDemuxForTest, instead of
creating a separate thread, we are just reusing the current thread for reading
from demuxer. We call stream->Read on line 334 and then call
MessageLoop::current()->Run on line 336, that blocks until we get a DemuxReadCB
and call QuitWhenIdle. I.e. this is just a way to convert asynchronous
DemuxerStream interface into synchronous FFmpegDemuxForTest API.

[gunsch, 2014-10-03 00:27:05]

PTAL

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
File chromecast/media/cma/test/DEPS (right):

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
chromecast/media/cma/test/DEPS:4: "+third_party/ffmpeg",
On 2014/10/03 00:06:22, damienv1 wrote:
> Only direct dependencies should be needed, right ?

There was an include on "third_party/ffmpeg/ffmpeg_stubs.h", but it looks like
it wasn't actually necessary. Removed the include and the DEPS line.

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:280:
base::MessageLoop::current()->QuitWhenIdle();
On 2014/10/03 00:06:22, damienv1 wrote:
> I don't think that's the right location to call QuitWhenIdle (should not be
done
> in this file).
> You should check with Sergey as to why it was done here (don't recall the
> reason), but that does not belong logically to this file.

Looks like using a RunLoop would be preferred to making explicit calls to
MessageLoop, at the very least. Separated it so it's only in the caller and not
here. Thoughts?

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
File chromecast/media/cma/test/frame_segmenter_for_test.h (right):

https://codereview.chromium.org/609383004/diff/20001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.h:48: 
On 2014/10/03 00:06:23, damienv1 wrote:
> nit: remove last line.

Done.

[damienv1, 2014-10-03 05:42:10]

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:17: #include
"media/ffmpeg/ffmpeg_common.h"
Do we need this include ?

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:269: void
FFDemuxNeedKeyCB(const std::string& type,
nit: This is usually the style used to typedef a callback.
I would rather have: OnDemuxerNeedKey.

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:274: void
DemuxReadCB(BufferList* buffer_list,
Same: OnNewBuffer.

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:314: &data_source,
Alignment.

[gunsch, 2014-10-03 16:26:17]

PTAL, thanks!

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:17: #include
"media/ffmpeg/ffmpeg_common.h"
On 2014/10/03 05:42:09, damienv1 wrote:
> Do we need this include ?

Huh, apparently not. That reduces the DEPS footprint, too.

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:269: void
FFDemuxNeedKeyCB(const std::string& type,
On 2014/10/03 05:42:09, damienv1 wrote:
> nit: This is usually the style used to typedef a callback.
> I would rather have: OnDemuxerNeedKey.

Done.

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:274: void
DemuxReadCB(BufferList* buffer_list,
On 2014/10/03 05:42:09, damienv1 wrote:
> Same: OnNewBuffer.

Done.

https://codereview.chromium.org/609383004/diff/60001/chromecast/media/cma/tes...
chromecast/media/cma/test/frame_segmenter_for_test.cc:314: &data_source,
On 2014/10/03 05:42:09, damienv1 wrote:
> Alignment.

Done.

[damienv1, 2014-10-03 16:48:50]

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:32: size_t mp3_bitrate[] =
{
Should rather use "int" instead of size_t here.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:34: size_t
mp3_sample_rate[] = { 44100, 48000, 32000, 0 };
Same here.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:87:
std::list<scoped_refptr<DecoderBufferBase> > Mp3SegmenterForTest(
nit: Use BufferList for the return type.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:90:
std::list<scoped_refptr<DecoderBufferBase> > audio_frames;
BufferList

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:134:
std::list<scoped_refptr<DecoderBufferBase> > video_frames;
BufferList

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
File chromecast/media/cma/test/frame_segmenter_for_test.h (right):

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.h:30: typedef std::list<
scoped_refptr<DecoderBufferBase> > BufferList;
nit1: Can you define it before and also use it for Mp3SegmenterForTest and
H264SegmenterForTest ?

nit2: no space needed between "std::list<" and "scoped_refptr"

[gunsch, 2014-10-03 16:52:53]

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:32: size_t mp3_bitrate[] =
{
On 2014/10/03 16:48:50, damienv1 wrote:
> Should rather use "int" instead of size_t here.

Done.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:34: size_t
mp3_sample_rate[] = { 44100, 48000, 32000, 0 };
On 2014/10/03 16:48:50, damienv1 wrote:
> Same here.

Done.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:87:
std::list<scoped_refptr<DecoderBufferBase> > Mp3SegmenterForTest(
On 2014/10/03 16:48:50, damienv1 wrote:
> nit: Use BufferList for the return type.

Done.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:90:
std::list<scoped_refptr<DecoderBufferBase> > audio_frames;
On 2014/10/03 16:48:50, damienv1 wrote:
> BufferList

Done.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.cc:134:
std::list<scoped_refptr<DecoderBufferBase> > video_frames;
On 2014/10/03 16:48:50, damienv1 wrote:
> BufferList

Done.

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
File chromecast/media/cma/test/frame_segmenter_for_test.h (right):

https://codereview.chromium.org/609383004/diff/100001/chromecast/media/cma/te...
chromecast/media/cma/test/frame_segmenter_for_test.h:30: typedef std::list<
scoped_refptr<DecoderBufferBase> > BufferList;
On 2014/10/03 16:48:50, damienv1 wrote:
> nit1: Can you define it before and also use it for Mp3SegmenterForTest and
> H264SegmenterForTest ?
> 
> nit2: no space needed between "std::list<" and "scoped_refptr"

Done.

[damienv1, 2014-10-03 17:23:02]

lgtm

[gunsch, 2014-10-03 19:36:27]

xhwang@: could you take a look? need OWNERS approval for "media/filters" dep,
but restricting it only to our "test" folder.

[xhwang, 2014-10-03 19:56:29]

the DEPS change lgtm

[gunsch, 2014-10-03 20:11:15]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-10-03 20:12:13]

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

[commit-bot: I haz the power, 2014-10-03 20:35:51]

Committed patchset #7 (id:120001) as a2fb57778338d0d33589bbc6881fb56b97dd328b

[commit-bot: I haz the power, 2014-10-03 20:36:19]

Patchset 7 (id:??) landed as
https://crrev.com/794810208e249261735b14e682b763ce3aa2d614
Cr-Commit-Position: refs/heads/master@{#298090}