Introducing DecoderBuffer and general Buffer cleanup.

Introducing DecoderBuffer and general Buffer cleanup.

FFmpeg expects data to be padded and aligned in a certain way. It's
currently possible to do this incorrectly and introduce dangerous issues.

I enforce padding and alignment by introducing a new Buffer called
DecoderBuffer and forcing DemuxerStream::Read to only accept it for
transfer into decoders.

DecoderBuffer allocates all memory through av_malloc (which takes care of
alignment) with the appropriate padding size (except for Android, which
doesn't care about this).

Along the way it was necessary to clean up a large smattering of code to
replace usage of DataBuffer with DecoderBuffer.

I've rolled in several cleanup actions as well:
- Moved DecryptConfig from Buffer to DecoderBuffer.
- Replaced AVPacketBuffer and av_dup_packet with a DecoderBuffer::CopyFrom.
- Fixed a resultant issue with FFmpegBitStreamConverter after removing the
  av_dup_packet functionality.  Removed some unsupported bitstream filters.
- Reduce TestDataUtil::ReadTestDataFile() to a single method returning a
  DecoderBuffer so unit tests will always have safe buffers.
- Replace new DataBuffer(0)/new DecoderBuffer(0) w/
  DecoderBuffer::CreateEOSBuffer.
- Remove extraneous IsEndOfStream check from FFmpegAudioDecoder.

BUG=129843
TEST=media_unittests + valgrind, layout tests.

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

[DaleCurtis, 2012-05-26 00:17:40]

PTAL guys.  Two sets of eyes would be nice.  Thanks!

[scherkus (not reviewing), 2012-05-26 01:36:32]

nice!

first round of comments comin' right at ya

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc
File media/base/data_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc#n...
media/base/data_buffer.cc:29: // If Initialize fails data_size_ == 0 and this
becomes a no-op.
comment not needed -- it's impossible for Initialize() to fail

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc#n...
media/base/data_buffer.cc:39: data_.reset(NULL);
s/NULL//

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc#n...
media/base/data_buffer.cc:44: CHECK(data_.get()) << "DataBuffer ctor failed to
allocate memory";
I noticed you moved this CHECK around but in fact we don't need it as we crash
on OOM

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:22: // If Initialize fails buffer_size_ == 0 and
this becomes a no-op.
not needed

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:45: CHECK(data_.get()) << "DecoderBuffer ctor
failed to allocate memory";
not needed

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:49: memset(data_.get() + buffer_size_, 0,
FF_INPUT_BUFFER_PADDING_SIZE);
can move this up into other #if section

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:20: #include "media/ffmpeg/ffmpeg_common.h"
this header file should only be included by .cc files as it pulls in the FFmpeg
header files and pollutes the namespace

It looks like the only reason it's included is for ScopedPtrAVFree but I'd
prefer to use av_malloc/av_free inside this class (with #if guards for android)
instead of having ffmpeg_common.h included here (we're not doing anything tricky
involve complicated scopes inside this class so we're not losing out on much by
not using it)

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
File media/base/decoder_buffer_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:29: scoped_refptr<DecoderBuffer> buffer2 =
DecoderBuffer::CopyFrom(
it looks like these two CopyFrom() tests be in a separate CopyFrom() tests --
also if you wanted to be sure about a copy you can assert that
buffer2->GetData() ptr != kData

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:35: #if !defined(OS_ANDROID)
then for a PaddingAlignment test I would #if the whole test so it doesn't even
run on android

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:38: memset(
since the area past is supposed to be zeros and because writing doesn't always
trigger a crash, how about writing a for loop that goes from GetWritableData() +
kDataSize and writes+reads 0xff?

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:41: // These alignments will need to be
updated to match FFmpeg when it changes.
OOC is it expected to change?

http://codereview.chromium.org/10447035/diff/7002/media/base/test_data_util.cc
File media/base/test_data_util.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/test_data_util.c...
media/base/test_data_util.cc:41: CHECK(file_size == file_util::ReadFile(
CHECK_EQ?

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:20: // Return a scoped_refptr to a Buffer object
with the decrypted data on success.
docs

also I'd drop the "scoped_reptr" mentions

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:14: #include "media/base/decoder_buffer.h"
fwd declare instead? (i.e., replace Buffer fwd decl)

http://codereview.chromium.org/10447035/diff/7002/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:118: scoped_refptr<DecoderBuffer> info;
nice cleanup!

http://codereview.chromium.org/10447035/diff/7002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:167: +
video_content_encodings->GetDataSize()
indenting here is funny

maybe drop video_track_entry->GetDataSize() to next line then do 4-space indent
from (

http://codereview.chromium.org/10447035/diff/7002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:168: - kVideoTrackEntryHeaderSize);
also binary operators go @ the end of the previous line

http://codereview.chromium.org/10447035/diff/7002/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (left):

http://codereview.chromium.org/10447035/diff/7002/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:271:
TEST_F(FFmpegVideoDecoderTest, DecodeFrame_0ByteFrame) {
I'd ping acolwell to see if he can remember why we had this

http://codereview.chromium.org/10447035/diff/7002/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/10447035/diff/7002/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:12: #include "media/base/decoder_buffer.h"
fwd decl instead?

[Ami GONE FROM CHROMIUM, 2012-05-26 06:07:22]

I skimmed the API changes and nothing jumped out at me.
I'll wait till the scherkus review settles down before looking more carefully,
unless one of you wants me to look sooner.

[DaleCurtis, 2012-05-29 21:17:01]

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc
File media/base/data_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc#n...
media/base/data_buffer.cc:29: // If Initialize fails data_size_ == 0 and this
becomes a no-op.
On 2012/05/26 01:36:32, scherkus wrote:
> comment not needed -- it's impossible for Initialize() to fail

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc#n...
media/base/data_buffer.cc:39: data_.reset(NULL);
On 2012/05/26 01:36:32, scherkus wrote:
> s/NULL//

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/data_buffer.cc#n...
media/base/data_buffer.cc:44: CHECK(data_.get()) << "DataBuffer ctor failed to
allocate memory";
On 2012/05/26 01:36:32, scherkus wrote:
> I noticed you moved this CHECK around but in fact we don't need it as we crash
> on OOM

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:22: // If Initialize fails buffer_size_ == 0 and
this becomes a no-op.
On 2012/05/26 01:36:32, scherkus wrote:
> not needed

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:45: CHECK(data_.get()) << "DecoderBuffer ctor
failed to allocate memory";
On 2012/05/26 01:36:32, scherkus wrote:
> not needed

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:49: memset(data_.get() + buffer_size_, 0,
FF_INPUT_BUFFER_PADDING_SIZE);
On 2012/05/26 01:36:32, scherkus wrote:
> can move this up into other #if section

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:20: #include "media/ffmpeg/ffmpeg_common.h"
On 2012/05/26 01:36:32, scherkus wrote:
> this header file should only be included by .cc files as it pulls in the
FFmpeg
> header files and pollutes the namespace
> 
> It looks like the only reason it's included is for ScopedPtrAVFree but I'd
> prefer to use av_malloc/av_free inside this class (with #if guards for
android)
> instead of having ffmpeg_common.h included here (we're not doing anything
tricky
> involve complicated scopes inside this class so we're not losing out on much
by
> not using it)

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
File media/base/decoder_buffer_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:29: scoped_refptr<DecoderBuffer> buffer2 =
DecoderBuffer::CopyFrom(
On 2012/05/26 01:36:32, scherkus wrote:
> it looks like these two CopyFrom() tests be in a separate CopyFrom() tests --
> also if you wanted to be sure about a copy you can assert that
> buffer2->GetData() ptr != kData

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:35: #if !defined(OS_ANDROID)
On 2012/05/26 01:36:32, scherkus wrote:
> then for a PaddingAlignment test I would #if the whole test so it doesn't even
> run on android

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:38: memset(
On 2012/05/26 01:36:32, scherkus wrote:
> since the area past is supposed to be zeros and because writing doesn't always
> trigger a crash, how about writing a for loop that goes from GetWritableData()
+
> kDataSize and writes+reads 0xff?

Done.

http://codereview.chromium.org/10447035/diff/7002/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:41: // These alignments will need to be
updated to match FFmpeg when it changes.
On 2012/05/26 01:36:32, scherkus wrote:
> OOC is it expected to change?

Probably if we ever get 512bit SIMD instructions :)

http://codereview.chromium.org/10447035/diff/7002/media/base/test_data_util.cc
File media/base/test_data_util.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/base/test_data_util.c...
media/base/test_data_util.cc:41: CHECK(file_size == file_util::ReadFile(
On 2012/05/26 01:36:32, scherkus wrote:
> CHECK_EQ?

Done.

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:20: // Return a scoped_refptr to a Buffer object
with the decrypted data on success.
On 2012/05/26 01:36:32, scherkus wrote:
> docs
> 
> also I'd drop the "scoped_reptr" mentions

Done.

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10447035/diff/7002/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:14: #include "media/base/decoder_buffer.h"
On 2012/05/26 01:36:32, scherkus wrote:
> fwd declare instead? (i.e., replace Buffer fwd decl)

Done.

http://codereview.chromium.org/10447035/diff/7002/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/7002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:167: +
video_content_encodings->GetDataSize()
On 2012/05/26 01:36:32, scherkus wrote:
> indenting here is funny
> 
> maybe drop video_track_entry->GetDataSize() to next line then do 4-space
indent
> from (

Done.

http://codereview.chromium.org/10447035/diff/7002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:168: - kVideoTrackEntryHeaderSize);
On 2012/05/26 01:36:32, scherkus wrote:
> also binary operators go @ the end of the previous line

Done.

http://codereview.chromium.org/10447035/diff/7002/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (left):

http://codereview.chromium.org/10447035/diff/7002/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:271:
TEST_F(FFmpegVideoDecoderTest, DecodeFrame_0ByteFrame) {
On 2012/05/26 01:36:32, scherkus wrote:
> I'd ping acolwell to see if he can remember why we had this

Per acolwell: "The 0-byte frame test was there to make sure FFmpeg didn't do
anything wacky with Theora 0-byte frames. In Theora they encode a 0 byte frame
to indicate that the frame should match the previous one"

Testing shows my new code broke this behavior, I've modified my implementation
such that 0-byte buffers are acceptable and constructing negatived buffers or
copying data from NULL result in NULL data sections.  As such I've undone this
change.

http://codereview.chromium.org/10447035/diff/7002/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/10447035/diff/7002/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:12: #include "media/base/decoder_buffer.h"
On 2012/05/26 01:36:32, scherkus wrote:
> fwd decl instead?

Done.

[scherkus (not reviewing), 2012-05-30 00:45:19]

few more nits

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:62: return make_scoped_refptr(new
DecoderBuffer(-1));
hrm.. doesn't this mean clients can call DataBuffer(-1) instead of this method?

I think I mentioned this somewhere else where it'd be preferable to crash on bad
input (i.e., DataBuffer(-1), CopyFrom(NULL, 0) -- how can I copy a NULL
pointer?), even it means we have to duplicate a few lines of code

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:25: // is set to a NULL ptr.  Buffer will be padded
and aligned as necessary.
I made a comment about this elsewhere

"data_ is set to a NULL ptr" is equivalent to calling CreateEOSBuffer() -- we
should disallow sizes < 0 and force clients to be explicit that they want an EOS
buffer

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:32: // Create a DecoderBuffer indicating we've
reached End Of Stream.
s/we've reached End Of Stream/end of stream/

also comment on what we can expect from the other accessors (i.e., GetData() ==
NULL)

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
File media/base/decoder_buffer_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:34: EXPECT_TRUE(buffer->IsEndOfStream());
what about the other methods?

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:38: scoped_refptr<DecoderBuffer>
buffer(DecoderBuffer::CopyFrom(NULL, 0));
does production code use CopyFrom(NULL, 0) today?

I'd assume we're CHECK-fail here

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:66: EXPECT_EQ((buffer2->GetData() +
kDataSize)[i], 0);
fix indent

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:68: const uint8 kFillChar = 0xFF;
I'd comment here that we're attempting to write here to make sure we don't trip
up under valgrind

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:73: EXPECT_EQ((buffer2->GetData() +
kDataSize)[i], kFillChar);
fix indent

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:83: EXPECT_EQ(0U,
reinterpret_cast<uintptr_t>(
nit: most (all?) of our code uses lower-case suffixes (U -> u)

http://codereview.chromium.org/10447035/diff/74/media/filters/ffmpeg_video_de...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/74/media/filters/ffmpeg_video_de...
media/filters/ffmpeg_video_decoder_unittest.cc:273: scoped_refptr<DecoderBuffer>
zero_byte_buffer = new DecoderBuffer(0);
omg this diff is hilarious haha

[DaleCurtis, 2012-05-30 02:28:25]

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:62: return make_scoped_refptr(new
DecoderBuffer(-1));
On 2012/05/30 00:45:19, scherkus wrote:
> hrm.. doesn't this mean clients can call DataBuffer(-1) instead of this
method?
> 
> I think I mentioned this somewhere else where it'd be preferable to crash on
bad
> input (i.e., DataBuffer(-1), CopyFrom(NULL, 0) -- how can I copy a NULL
> pointer?), even it means we have to duplicate a few lines of code

Done. Public interfaces DCHECK() this behavior. Internally, the call is still
the same though.

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:25: // is set to a NULL ptr.  Buffer will be padded
and aligned as necessary.
On 2012/05/30 00:45:19, scherkus wrote:
> I made a comment about this elsewhere
> 
> "data_ is set to a NULL ptr" is equivalent to calling CreateEOSBuffer() -- we
> should disallow sizes < 0 and force clients to be explicit that they want an
EOS
> buffer

Done. Public interfaces DCHECK() this behavior.

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:32: // Create a DecoderBuffer indicating we've
reached End Of Stream.
On 2012/05/30 00:45:19, scherkus wrote:
> s/we've reached End Of Stream/end of stream/
> 
> also comment on what we can expect from the other accessors (i.e., GetData()
==
> NULL)

Done.

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
File media/base/decoder_buffer_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:34: EXPECT_TRUE(buffer->IsEndOfStream());
On 2012/05/30 00:45:19, scherkus wrote:
> what about the other methods?

Added checks for GetData() and GetDataSize().

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:38: scoped_refptr<DecoderBuffer>
buffer(DecoderBuffer::CopyFrom(NULL, 0));
On 2012/05/30 00:45:19, scherkus wrote:
> does production code use CopyFrom(NULL, 0) today?
> 
> I'd assume we're CHECK-fail here

I've changed FFmpegDemuxer::StreamHasEnded() so that it doesn't create NULL
packets any more and internalized the end of stream buffer creation which allows
the code to DCHECK on data == NULL and size < 0.

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:66: EXPECT_EQ((buffer2->GetData() +
kDataSize)[i], 0);
On 2012/05/30 00:45:19, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:68: const uint8 kFillChar = 0xFF;
On 2012/05/30 00:45:19, scherkus wrote:
> I'd comment here that we're attempting to write here to make sure we don't
trip
> up under valgrind

Done.

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:73: EXPECT_EQ((buffer2->GetData() +
kDataSize)[i], kFillChar);
On 2012/05/30 00:45:19, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/10447035/diff/74/media/base/decoder_buffer_uni...
media/base/decoder_buffer_unittest.cc:83: EXPECT_EQ(0U,
reinterpret_cast<uintptr_t>(
On 2012/05/30 00:45:19, scherkus wrote:
> nit: most (all?) of our code uses lower-case suffixes (U -> u)

Done.

[scherkus (not reviewing), 2012-05-30 02:48:42]

LGTM w/ some nits and musings about the 1-arg DecodeBuffer ctor

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:26: explicit DecoderBuffer(int buffer_size);
while doing a final review pass it appears that there are no real production
users of this public ctor, which makes me wonder whether it should remain public

Don't let that stop you from committing the change -- just something to think
about!

Side-bar: my dealings with Go has me on a big "ctors (especially overloaded
ones) don't produce self-documenting code!" kick.  It's sort of a contrived
case, but imagine if you were new to the code and had to compare these two
snippets of code:
  1) db = new DecoderBuffer(1024);
  2) db = DecoderBuffer::Allocate(1024);

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer_u...
File media/base/decoder_buffer_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:15: TEST(DecoderBufferTest, Constructors)
{
these tests use the 1-arg ctor out of convenience -- I wonder if we should
privatize the ctor

http://codereview.chromium.org/10447035/diff/6069/media/base/test_data_util.cc
File media/base/test_data_util.cc (right):

http://codereview.chromium.org/10447035/diff/6069/media/base/test_data_util.c...
media/base/test_data_util.cc:40: scoped_refptr<DecoderBuffer> buffer(new
DecoderBuffer(file_size));
here's another

http://codereview.chromium.org/10447035/diff/6069/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10447035/diff/6069/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:192: it->Run(scoped_refptr<DecoderBuffer>());
OOC can this simply be NULL?

I believe a scoped_refptr<T> will be created automatically

http://codereview.chromium.org/10447035/diff/6069/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10447035/diff/6069/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:694: scoped_ptr_malloc<AVPacket,
ScopedPtrAVFreePacket>());
it saddens me that we can't simply say "NULL" but this is the strongly-typed
world we live in :(

http://codereview.chromium.org/10447035/diff/6069/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10447035/diff/6069/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:273: scoped_refptr<DecoderBuffer>
zero_byte_buffer = new DecoderBuffer(0);
here's another

[DaleCurtis, 2012-05-30 03:20:11]

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:26: explicit DecoderBuffer(int buffer_size);
On 2012/05/30 02:48:42, scherkus wrote:
> while doing a final review pass it appears that there are no real production
> users of this public ctor, which makes me wonder whether it should remain
public
> 
> Don't let that stop you from committing the change -- just something to think
> about!
> 
> Side-bar: my dealings with Go has me on a big "ctors (especially overloaded
> ones) don't produce self-documenting code!" kick.  It's sort of a contrived
> case, but imagine if you were new to the code and had to compare these two
> snippets of code:
>   1) db = new DecoderBuffer(1024);
>   2) db = DecoderBuffer::Allocate(1024);

We should keep this constructor since it's impossible to allocate a buffer and
then write data into it otherwise.  You would always need to allocate a
secondary buffer, read your data into it, and then use the CopyFrom()
constructor to copy the data into DecoderBuffer.

Neither method is any clearer than the other to me.  Without context of what a
DecoderBuffer is I don't know if it's allocating 1024 buffers, a 1024 byte
buffer, or some random other quantity.

You could start using Java-style DecoderBuffer::AllocateNewBufferInBytes(1024)
style naming, but I suspect that would only be helpful in trivial cases like
this.

http://codereview.chromium.org/10447035/diff/6069/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10447035/diff/6069/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:192: it->Run(scoped_refptr<DecoderBuffer>());
On 2012/05/30 02:48:42, scherkus wrote:
> OOC can this simply be NULL?
> 
> I believe a scoped_refptr<T> will be created automatically

Yup. Done.

[DaleCurtis, 2012-05-30 03:50:52]

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:26: explicit DecoderBuffer(int buffer_size);
On 2012/05/30 03:20:11, DaleCurtis wrote:
> On 2012/05/30 02:48:42, scherkus wrote:
> > while doing a final review pass it appears that there are no real production
> > users of this public ctor, which makes me wonder whether it should remain
> public
> > 
> > Don't let that stop you from committing the change -- just something to
think
> > about!
> > 
> > Side-bar: my dealings with Go has me on a big "ctors (especially overloaded
> > ones) don't produce self-documenting code!" kick.  It's sort of a contrived
> > case, but imagine if you were new to the code and had to compare these two
> > snippets of code:
> >   1) db = new DecoderBuffer(1024);
> >   2) db = DecoderBuffer::Allocate(1024);
> 
> We should keep this constructor since it's impossible to allocate a buffer and
> then write data into it otherwise.  You would always need to allocate a
> secondary buffer, read your data into it, and then use the CopyFrom()
> constructor to copy the data into DecoderBuffer.
> 
> Neither method is any clearer than the other to me.  Without context of what a
> DecoderBuffer is I don't know if it's allocating 1024 buffers, a 1024 byte
> buffer, or some random other quantity.
> 
> You could start using Java-style DecoderBuffer::AllocateNewBufferInBytes(1024)
> style naming, but I suspect that would only be helpful in trivial cases like
> this.

Also, Ronald has a use case for this constructor coming up in
http://codereview.chromium.org/10451051/

[scherkus (not reviewing), 2012-05-30 04:32:26]

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/6069/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:26: explicit DecoderBuffer(int buffer_size);
Gah I think you're reading too much into my sidebar / musings on ctors!

I even refrained from mentioning Java because I didn't want to give the
impression that I'd prefer something like DB::ANBIB()!!! (FWIW that name would
be un-Go-like as well)

On 2012/05/30 03:20:11, DaleCurtis wrote:
> On 2012/05/30 02:48:42, scherkus wrote:
> > while doing a final review pass it appears that there are no real production
> > users of this public ctor, which makes me wonder whether it should remain
> public
> > 
> > Don't let that stop you from committing the change -- just something to
think
> > about!
> > 
> > Side-bar: my dealings with Go has me on a big "ctors (especially overloaded
> > ones) don't produce self-documenting code!" kick.  It's sort of a contrived
> > case, but imagine if you were new to the code and had to compare these two
> > snippets of code:
> >   1) db = new DecoderBuffer(1024);
> >   2) db = DecoderBuffer::Allocate(1024);
> 
> We should keep this constructor since it's impossible to allocate a buffer and
> then write data into it otherwise.  You would always need to allocate a
> secondary buffer, read your data into it, and then use the CopyFrom()
> constructor to copy the data into DecoderBuffer.
> 
> Neither method is any clearer than the other to me.  Without context of what a
> DecoderBuffer is I don't know if it's allocating 1024 buffers, a 1024 byte
> buffer, or some random other quantity.
> 
> You could start using Java-style DecoderBuffer::AllocateNewBufferInBytes(1024)
> style naming, but I suspect that would only be helpful in trivial cases like
> this.

[Ami GONE FROM CHROMIUM, 2012-05-30 05:19:27]

Basically LGTM.

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:24: buffer_size_(data == NULL ? -1 : buffer_size) {
What's up with this -1?
(and why isn't it sending memcpy below into a tizzy interpreting it as a
kuint64max on x64?)

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:64: return make_scoped_refptr(new
DecoderBuffer(NULL, -1));
Again, I think -1 is just asking for trouble.  Why not 0?

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:47: DecoderBuffer(const uint8* data, int size);
doco copying behavior

http://codereview.chromium.org/10447035/diff/4144/media/base/test_data_util.cc
File media/base/test_data_util.cc (right):

http://codereview.chromium.org/10447035/diff/4144/media/base/test_data_util.c...
media/base/test_data_util.cc:33: 
In case you're unaware of it: file_util::ReadFileToString().

http://codereview.chromium.org/10447035/diff/4144/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (left):

http://codereview.chromium.org/10447035/diff/4144/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:46: // TODO(xhwang): Implement a string based
Buffer implementation to avoid
Why drop the spirit of this TODO?

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:83: // Convert if the packet if there is
bitstream filter.
s/if the/the/

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:83: // Convert if the packet if there is
bitstream filter.
s/is/is a/

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:91: // into memory we control.
This is what rbultje@ is fixing, right?

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:212: }
I realize this isn't new, but I wonder if this should somehow indicate if the
codec is unsupported.

[DaleCurtis, 2012-05-30 17:33:47]

Thanks guys!

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:24: buffer_size_(data == NULL ? -1 : buffer_size) {
On 2012/05/30 05:19:27, Ami Fischman wrote:
> What's up with this -1?
> (and why isn't it sending memcpy below into a tizzy interpreting it as a
> kuint64max on x64?)

Ensures if a NULL buffer is passed in with 0 size the proper Initialize()
behavior is still taken.  As I wrote this I realized the code can be directly
moved here now that we're DCHECK'ing the other constructors.

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:64: return make_scoped_refptr(new
DecoderBuffer(NULL, -1));
On 2012/05/30 05:19:27, Ami Fischman wrote:
> Again, I think -1 is just asking for trouble.  Why not 0?

Done.

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10447035/diff/4144/media/base/decoder_buffer.h...
media/base/decoder_buffer.h:47: DecoderBuffer(const uint8* data, int size);
On 2012/05/30 05:19:27, Ami Fischman wrote:
> doco copying behavior

Done.

http://codereview.chromium.org/10447035/diff/4144/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (left):

http://codereview.chromium.org/10447035/diff/4144/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.cc:46: // TODO(xhwang): Implement a string based
Buffer implementation to avoid
On 2012/05/30 05:19:27, Ami Fischman wrote:
> Why drop the spirit of this TODO?

Good point. Changed.

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:83: // Convert if the packet if there is
bitstream filter.
On 2012/05/30 05:19:27, Ami Fischman wrote:
> s/if the/the/

Done.

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:83: // Convert if the packet if there is
bitstream filter.
On 2012/05/30 05:19:27, Ami Fischman wrote:
> s/is/is a/

Done.

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:91: // into memory we control.
On 2012/05/30 05:19:27, Ami Fischman wrote:
> This is what rbultje@ is fixing, right?

I don't think so, here we're dealing with AVPacket data collected from
av_read_frame which references internal FFmpeg memory which might be destroyed
on the next av_read_frame.

Ronald's fix deals with the extra copy we do after avcodec_decode_video()
decodes this AVPacket data into an AVFrame.

Currently, we copy the AVFrame into a VideoFrame post-decode.  His change makes
it so that we provide FFmpeg an AVFrame during decode which is actually the
backing for a VideoFrame we can use.

http://codereview.chromium.org/10447035/diff/4144/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:212: }
On 2012/05/30 05:19:27, Ami Fischman wrote:
> I realize this isn't new, but I wonder if this should somehow indicate if the
> codec is unsupported.

Done.

[commit-bot: I haz the power, 2012-05-30 17:34:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10447035/7012

[Ami GONE FROM CHROMIUM, 2012-05-30 17:38:30]

FYI

http://codereview.chromium.org/10447035/diff/7012/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/7012/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:18: DCHECK(buffer_size >= 0);
FWIW, DCHECK_GE.

http://codereview.chromium.org/10447035/diff/7012/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:26: if (data == NULL || buffer_size_ < 0) {
In the public entry points you DCHECK non-negativity, but in this private place
you defend against negativity.  That's confusing (and useless).

[Ami GONE FROM CHROMIUM, 2012-05-30 17:38:36]

FYI

[commit-bot: I haz the power, 2012-05-30 17:49:17]

Try job failure for 10447035-7012 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[DaleCurtis, 2012-05-30 17:56:17]

http://codereview.chromium.org/10447035/diff/7012/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10447035/diff/7012/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:18: DCHECK(buffer_size >= 0);
On 2012/05/30 17:38:31, Ami Fischman wrote:
> FWIW, DCHECK_GE.

Done. CQ failed seek_tester, your gain!

http://codereview.chromium.org/10447035/diff/7012/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:26: if (data == NULL || buffer_size_ < 0) {
On 2012/05/30 17:38:31, Ami Fischman wrote:
> In the public entry points you DCHECK non-negativity, but in this private
place
> you defend against negativity.  That's confusing (and useless).

Good point. Fixed.

[commit-bot: I haz the power, 2012-05-30 17:57:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10447035/15044

[commit-bot: I haz the power, 2012-05-30 18:29:54]

Try job failure for 10447035-15044 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-30 18:46:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10447035/14014

[commit-bot: I haz the power, 2012-05-30 19:28:12]

Try job failure for 10447035-14014 (retry) on mac_rel for step
"media_unittests".
It's a second try, previously, step "media_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-31 01:07:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10447035/14014

[commit-bot: I haz the power, 2012-05-31 02:21:44]

Try job failure for 10447035-14014 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-31 18:22:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10447035/14014

[commit-bot: I haz the power, 2012-05-31 20:18:32]

Try job failure for 10447035-14014 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[DaleCurtis, 2012-05-31 20:36:35]

On 2012/05/31 20:18:32, I haz the power (commit-bot) wrote:
> Try job failure for 10447035-14014 (retry) on win_rel for step "compile"
> (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Landed manually, http://crrev.com/139857