Tighten up media::DecoderBuffer API contract for end of stream buffers (round 2).

Tighten up media::DecoderBuffer API contract for end of stream buffers (round 2).

Folks shouldn't be calling methods on end of stream buffers since the definition is undefined.

This time with unit test fixes!

TBR=yzshen

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

[scherkus (not reviewing), 2013-01-26 00:14:44]

Can you take another look at this CL? I had to update a bunch of CDM stuff.

https://codereview.chromium.org/11929015/diff/4017/media/base/stream_parser_b...
File media/base/stream_parser_buffer.cc (right):

https://codereview.chromium.org/11929015/diff/4017/media/base/stream_parser_b...
media/base/stream_parser_buffer.cc:39: SetDuration(kNoTimestamp());
I wonder if kNoTimestamp() should be the default value in the superclass

Today it's zero, which is a valid (and common!) timestamp

[xhwang, 2013-01-28 16:35:20]

lgtm % comments

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/cdm_video_decoder.h (right):

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_video_decoder.h:27: // A null |compressed_frame|
is will attempt to flush the decoder of any
s/is will/will

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right):

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:46: // A null
|compressed_buffer| is will attempt to flush the decoder of any
ditto

https://codereview.chromium.org/11929015/diff/4017/media/base/stream_parser_b...
File media/base/stream_parser_buffer.cc (right):

https://codereview.chromium.org/11929015/diff/4017/media/base/stream_parser_b...
media/base/stream_parser_buffer.cc:39: SetDuration(kNoTimestamp());
On 2013/01/26 00:14:44, scherkus wrote:
> I wonder if kNoTimestamp() should be the default value in the superclass
> 
> Today it's zero, which is a valid (and common!) timestamp

Sounds good to me. But nothing says the default should be invalid :) At least we
need to make the behavior of these two classes the same.

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (left):

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:103: DCHECK(data_size) <<
"DecryptConfig is set on an empty buffer";
can we keep this DCHECK so that we won't miss 0-byte length buffer?

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:964: *resource = NULL;
looks like you found a bug! thanks!

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:968: const size_t data_size =
static_cast<size_t>(encrypted_buffer->GetDataSize());
move this down to 972 or 975?

[scherkus (not reviewing), 2013-01-28 18:28:37]

thanks!

TBR OWNERS yzshen@ for webkit/plugins/ppapi

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/cdm_video_decoder.h (right):

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_video_decoder.h:27: // A null |compressed_frame|
is will attempt to flush the decoder of any
On 2013/01/28 16:35:20, xhwang wrote:
> s/is will/will

Done.

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h (right):

https://codereview.chromium.org/11929015/diff/4016/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.h:46: // A null
|compressed_buffer| is will attempt to flush the decoder of any
On 2013/01/28 16:35:20, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/11929015/diff/4017/media/base/stream_parser_b...
File media/base/stream_parser_buffer.cc (right):

https://codereview.chromium.org/11929015/diff/4017/media/base/stream_parser_b...
media/base/stream_parser_buffer.cc:39: SetDuration(kNoTimestamp());
On 2013/01/28 16:35:20, xhwang wrote:
> On 2013/01/26 00:14:44, scherkus wrote:
> > I wonder if kNoTimestamp() should be the default value in the superclass
> > 
> > Today it's zero, which is a valid (and common!) timestamp
> 
> Sounds good to me. But nothing says the default should be invalid :) At least
we
> need to make the behavior of these two classes the same.

I'll add a TODO -- I *think* the reason why the ctors don't force you to set
timestamp+duration is because we were commonly using DecoderBuffer(0) for EOS
buffers ... but that's no longer a concern

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (left):

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:103: DCHECK(data_size) <<
"DecryptConfig is set on an empty buffer";
On 2013/01/28 16:35:20, xhwang wrote:
> can we keep this DCHECK so that we won't miss 0-byte length buffer?

Done.

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:964: *resource = NULL;
On 2013/01/28 16:35:20, xhwang wrote:
> looks like you found a bug! thanks!

:)

https://codereview.chromium.org/11929015/diff/4017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:968: const size_t data_size =
static_cast<size_t>(encrypted_buffer->GetDataSize());
On 2013/01/28 16:35:20, xhwang wrote:
> move this down to 972 or 975?

Done.

[commit-bot: I haz the power, 2013-01-28 18:28:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/11929015/4018

[commit-bot: I haz the power, 2013-01-28 23:36:01]

Change committed as 179212