Fix Issue 160529 in a nice way

Fix Issue 160529 in a nice way with unittest.
Original CL171023 by Song YeWen.

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=24016

[fbarchard, 2009-08-18 22:55:02]

A simplier fix for demuxer packet duplication, which leads to a simplier
unittest.
Also lint cleanup of unittest.

[scherkus (not reviewing), 2009-08-18 23:42:46]

Don't forget to credit Song YeWen and the link to the original CL in your change
description

http://codereview.chromium.org/174027/diff/15/16
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/174027/diff/15/16#newcode479
Line 479: // If a packets is returned by FFmpeg's av_parser_parse2()
packets -> packet

http://codereview.chromium.org/174027/diff/15/17
File media/filters/ffmpeg_demuxer_unittest.cc (left):

http://codereview.chromium.org/174027/diff/15/17#oldcode323
Line 323: EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_,
_)).WillOnce(NewPacket());
I imagine these should be replaced with expectations that av_dup_packet will be
called

Even if we don't want to set the expectations, mock_ffmpeg.cc will need to
implement av_dup_packet() so that media_unittests does not depend on FFmpeg
binaries (as a test, try deleting the DLLs and running media_unittests, you'll
should get a dependency not found dialog)

[fbarchard, 2009-08-19 19:08:31]

DupPackets are in the wrong spots.
Without DupPacket mock, the unittest fails if you delete the ffmpeg DLLs.
Good catch Andrew

http://codereview.chromium.org/174027/diff/15/16
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/174027/diff/15/16#newcode479
Line 479: // If a packets is returned by FFmpeg's av_parser_parse2()
On 2009/08/18 23:42:46, scherkus wrote:
> packets -> packet

Done.

http://codereview.chromium.org/174027/diff/23/1014
File media/filters/ffmpeg_demuxer_unittest.cc (left):

http://codereview.chromium.org/174027/diff/23/1014#oldcode324
Line 324: EXPECT_CALL(*MockFFmpeg::get(),
AVFreePacket(_)).WillOnce(FreePacket());
Add EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
	        .WillOnce(Return(0));

http://codereview.chromium.org/174027/diff/23/1014#oldcode497
Line 497: EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_,
_)).WillOnce(NewPacket());
need a duppacket for this too

http://codereview.chromium.org/174027/diff/23/1014#oldcode498
Line 498: EXPECT_CALL(*MockFFmpeg::get(),
AVFreePacket(_)).WillOnce(FreePacket());
Add EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
	        .WillOnce(Return(0));

http://codereview.chromium.org/174027/diff/23/1014#oldcode510
Line 510: EXPECT_CALL(*MockFFmpeg::get(),
AVFreePacket(_)).WillOnce(FreePacket());
Add EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
	        .WillOnce(Return(0));

http://codereview.chromium.org/174027/diff/23/1014
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/174027/diff/23/1014#newcode349
Line 349: .WillOnce(Return(0));
Remove this

[scherkus (not reviewing), 2009-08-19 19:35:30]

LGTM with one nit

you can submit after fixing

http://codereview.chromium.org/174027/diff/23/1017
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/174027/diff/23/1017#newcode169
Line 169: int av_dup_packet(AVPacket* packet) {
nit: this method should come after av_free() (following order in header file)

[fbarchard, 2009-08-20 22:30:10]

fixed read, but seek fails

http://codereview.chromium.org/174027/diff/23/1017
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/174027/diff/23/1017#newcode169
Line 169: int av_dup_packet(AVPacket* packet) {
On 2009/08/19 19:35:30, scherkus wrote:
> nit: this method should come after av_free() (following order in header file)

Done.

http://codereview.chromium.org/174027/diff/23/1014
File media/filters/ffmpeg_demuxer_unittest.cc (left):

http://codereview.chromium.org/174027/diff/23/1014#oldcode324
Line 324: EXPECT_CALL(*MockFFmpeg::get(),
AVFreePacket(_)).WillOnce(FreePacket());
On 2009/08/19 19:08:31, fbarchard wrote:
> Add EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
> 	        .WillOnce(Return(0));

Done.

http://codereview.chromium.org/174027/diff/23/1014#oldcode498
Line 498: EXPECT_CALL(*MockFFmpeg::get(),
AVFreePacket(_)).WillOnce(FreePacket());
On 2009/08/19 19:08:31, fbarchard wrote:
> Add EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
> 	        .WillOnce(Return(0));

Done.

http://codereview.chromium.org/174027/diff/23/1014#oldcode510
Line 510: EXPECT_CALL(*MockFFmpeg::get(),
AVFreePacket(_)).WillOnce(FreePacket());
On 2009/08/19 19:08:31, fbarchard wrote:
> Add EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
> 	        .WillOnce(Return(0));

Done.

http://codereview.chromium.org/174027/diff/23/1014
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/174027/diff/23/1014#newcode349
Line 349: .WillOnce(Return(0));
On 2009/08/19 19:08:31, fbarchard wrote:
> Remove this

Done.

[fbarchard, 2009-08-20 22:36:00]

seek demux inittest passing.

http://codereview.chromium.org/174027/diff/29/1023
File media/filters/ffmpeg_demuxer_unittest.cc (left):

http://codereview.chromium.org/174027/diff/29/1023#oldcode498
Line 498: EXPECT_CALL(*MockFFmpeg::get(),
AVFreePacket(_)).WillOnce(FreePacket());
mixed this one - needs a duppacket

[fbarchard, 2009-08-20 22:39:41]

What exactly does av_dup_packet do?
http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2005-June/002173.html

the buffer of an AVPacket is either used just for one packet and allocated 
with av_malloc() (lets call them non-shared packets) or its from somewhere 
else like part of another packet or a reuseable buffer (lets call these 
shared)
av_dup_packet() changes the shared buffer type AVPackets to non shared by 
malloc+memcpy

[Alpha Left Google, 2009-08-20 22:43:31]

http://codereview.chromium.org/174027/diff/1030/1031
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/174027/diff/1030/1031#newcode283
Line 283: DCHECK(stream < static_cast<int>(streams_.size()));
you can change this to DCHECK_LT too since you are making the change above.

http://codereview.chromium.org/174027/diff/1030/1031#newcode476
Line 476: DCHECK(packet->stream_index <
static_cast<int>(packet_streams_.size()));
you can change this to DCHECK_LT too since you are making the change above.

http://codereview.chromium.org/174027/diff/1030/1032
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/174027/diff/1030/1032#newcode599
Line 599: void (FFmpegDemuxerTest::*)(Buffer*),  // NOLINT
you can remove // NOLINT

[fbarchard, 2009-08-20 23:03:30]

fixed.  note that lint has a bug on class member function pointers, thinking its
a C style cast

http://codereview.chromium.org/174027/diff/1030/1031
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/174027/diff/1030/1031#newcode283
Line 283: DCHECK(stream < static_cast<int>(streams_.size()));
On 2009/08/20 22:43:31, Alpha wrote:
> you can change this to DCHECK_LT too since you are making the change above.
> 

Done.

http://codereview.chromium.org/174027/diff/1030/1031#newcode476
Line 476: DCHECK(packet->stream_index <
static_cast<int>(packet_streams_.size()));
On 2009/08/20 22:43:31, Alpha wrote:
> you can change this to DCHECK_LT too since you are making the change above.
> 

Done.

http://codereview.chromium.org/174027/diff/1030/1032
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/174027/diff/1030/1032#newcode599
Line 599: void (FFmpegDemuxerTest::*)(Buffer*),  // NOLINT
On 2009/08/20 22:43:31, Alpha wrote:
> you can remove // NOLINT
> 
not if we want it lint free.

[scherkus (not reviewing), 2009-08-20 23:14:45]

http://codereview.chromium.org/174027/diff/1030/1032
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/174027/diff/1030/1032#newcode599
Line 599: void (FFmpegDemuxerTest::*)(Buffer*),  // NOLINT
On 2009/08/20 23:03:30, fbarchard wrote:
> On 2009/08/20 22:43:31, Alpha wrote:
> > you can remove // NOLINT
> > 
> not if we want it lint free.
> 

I'm fine getting rid of the comment and having lint complain.. it's clearly a
bug

in fact sounds like we should submit a patch to the linter :)

[fbarchard, 2009-08-20 23:39:30]

fixed to lint issues in mock, and reintroduced one in demux unittest.

http://codereview.chromium.org/174027/diff/1030/1032
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/174027/diff/1030/1032#newcode599
Line 599: void (FFmpegDemuxerTest::*)(Buffer*),  // NOLINT
On 2009/08/20 23:14:46, scherkus wrote:
> On 2009/08/20 23:03:30, fbarchard wrote:
> > On 2009/08/20 22:43:31, Alpha wrote:
> > > you can remove // NOLINT
> > > 
> > not if we want it lint free.
> > 
> 
> I'm fine getting rid of the comment and having lint complain.. it's clearly a
> bug
> 
> in fact sounds like we should submit a patch to the linter :)

Done.

[fbarchard, 2009-08-21 18:48:46]

ping

[Alpha Left Google, 2009-08-21 18:51:55]

LGTM

[scherkus (not reviewing), 2009-08-21 18:53:51]

LGTM