demux duplicate data the same way as mp3 does.

demux duplicate data the same way as mp3 does.

BUG=16020
TEST=test with all media types and ensure there are no memory leaks are functional differences from previous version.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23455

[fbarchard, 2009-08-03 18:15:45]

This change deep copies all demux packets, which makes it safer for file formats
that change the data on a per frame basis.
The extra allocation is causing the unittest to fail and has been temporarily
disabled.

[Alpha Left Google, 2009-08-03 18:32:14]

The code itself looks good. But please fix the unit tests before checking in,
those are important tests that we cannot afford to lose..

[fbarchard, 2009-08-14 18:51:35]

gmock fixed, mp3 test disabled.

[scherkus (not reviewing), 2009-08-14 18:59:04]

change looks good, just style nits

http://codereview.chromium.org/160529/diff/2001/3001
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/160529/diff/2001/3001#newcode292
Line 292: #if 1
remove this

http://codereview.chromium.org/160529/diff/2001/3001#newcode363
Line 363: EXPECT_EQ(0, memcmp(kAudioData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
>80 chars, wrap at memcmp(

http://codereview.chromium.org/160529/diff/2001/3001#newcode380
Line 380: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
>80 chars, wrap at memcmp(

http://codereview.chromium.org/160529/diff/2001/3001#newcode420
Line 420: #endif
...and this

http://codereview.chromium.org/160529/diff/2001/3001#newcode525
Line 525: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
>80 chars, wrap at memcmp(

http://codereview.chromium.org/160529/diff/2001/3001#newcode554
Line 554: EXPECT_EQ(0, memcmp(kAudioData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
>80 chars, wrap at memcmp(

http://codereview.chromium.org/160529/diff/2001/3001#newcode564
Line 564: EXPECT_EQ(0, memcmp(kAudioData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
>80 chars, wrap at memcmp(

http://codereview.chromium.org/160529/diff/2001/3001#newcode574
Line 574: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
>80 chars, wrap at memcmp(

http://codereview.chromium.org/160529/diff/2001/3001#newcode584
Line 584: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
>80 chars, wrap at memcmp(

http://codereview.chromium.org/160529/diff/2001/3001#newcode592
Line 592: TEST_F(FFmpegDemuxerTest, DISABLED_MP3Hack) {
you can go ahead and delete the test instead of disabling

[fbarchard, 2009-08-14 19:02:30]

Done

http://codereview.chromium.org/160529/diff/2001/3001
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/160529/diff/2001/3001#newcode292
Line 292: #if 1
On 2009/08/14 18:59:04, scherkus wrote:
> remove this

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode363
Line 363: EXPECT_EQ(0, memcmp(kAudioData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
On 2009/08/14 18:59:04, scherkus wrote:
> >80 chars, wrap at memcmp(

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode380
Line 380: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
On 2009/08/14 18:59:04, scherkus wrote:
> >80 chars, wrap at memcmp(

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode420
Line 420: #endif
On 2009/08/14 18:59:04, scherkus wrote:
> ...and this

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode525
Line 525: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
On 2009/08/14 18:59:04, scherkus wrote:
> >80 chars, wrap at memcmp(

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode554
Line 554: EXPECT_EQ(0, memcmp(kAudioData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
On 2009/08/14 18:59:04, scherkus wrote:
> >80 chars, wrap at memcmp(

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode564
Line 564: EXPECT_EQ(0, memcmp(kAudioData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
On 2009/08/14 18:59:04, scherkus wrote:
> >80 chars, wrap at memcmp(

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode574
Line 574: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
On 2009/08/14 18:59:04, scherkus wrote:
> >80 chars, wrap at memcmp(

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode584
Line 584: EXPECT_EQ(0, memcmp(kVideoData, reader->buffer()->GetData(),
reader->buffer()->GetDataSize()));
On 2009/08/14 18:59:04, scherkus wrote:
> >80 chars, wrap at memcmp(

Done.

http://codereview.chromium.org/160529/diff/2001/3001#newcode592
Line 592: TEST_F(FFmpegDemuxerTest, DISABLED_MP3Hack) {
On 2009/08/14 18:59:04, scherkus wrote:
> you can go ahead and delete the test instead of disabling

Done.

[fbarchard, 2009-08-14 19:05:50]

indented memcmp parameters to memcmp(

[Alpha Left Google, 2009-08-14 19:07:48]

LGTM

[scherkus (not reviewing), 2009-08-14 19:48:19]

awesome!  lgtm!

[fbarchard, 2009-08-14 20:06:36]

Just waiting for tree to open.
Removed blank line.