Plumb support for audio sample formats.

Plumb support for audio sample formats.

- Introduces a new PP_DecryptedSampleInfo structure which
contains the AudioFormat for use when delivering samples.
- Plumbs the structure through to ContentDecryptorDelegate.
- Extends the ContentDecryptorDelegate code to handle planar
data.
- Cleans up FFmpegCdmAudioDecoder to remove 3 memcpy's in the
common case (1 decode per packet) and 2 in the uncommon (> 1
decode per packet).

BUG=169105
TEST=eme browsertests

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

[DaleCurtis, 2013-10-11 01:30:53]

dmichael: pepper stuff.
xhwang: media stuff.

Thanks in advance for your reviews!

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
ppapi/api/private/pp_content_decryptor.idl:256: [assert_size(32)]
Not sure where the extra 4 bytes comes from, it counts out to 28 by my hand
calculation... maybe padding?

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
ppapi/api/private/pp_content_decryptor.idl:271: uint32_t data_size;
Is this necessary? The size is already attached to the PP_Resource.  I just
followed what PP_DecryptedFrameInfo is doing...

[xhwang, 2013-10-11 22:45:16]

looks pretty good, thanks! Just a few comments.

https://codereview.chromium.org/26956002/diff/1/content/renderer/pepper/conte...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/26956002/diff/1/content/renderer/pepper/conte...
content/renderer/pepper/content_decryptor_delegate.cc:1042: int num_channel_ptrs
= 1;
A brief comment why the default is 1? Interleaved bla bla..

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
File media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:326: // into the frame buffer
instead of a multistep serialization approach.
Hmm, how much improvement does this approach buy us? It make the code a little
bit harder to follow. For example, we return early at l.383 for case at l.328;
and we have SetFrameBuffer on l.330 and l.386 which are pretty far away.

If the improvement is not trivial, can we wrap 355-377 to a helper function,
call that function and return early right after l.344?

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:365: decoded_audio_size /
av_frame_->channels;
shall we CHECK(decoded_audio_size % av_frame_->channels == 0) ? otherwise line
367 may overflow.

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
ppapi/api/private/pp_content_decryptor.idl:256: [assert_size(32)]
On 2013/10/11 01:30:53, DaleCurtis wrote:
> Not sure where the extra 4 bytes comes from, it counts out to 28 by my hand
> calculation... maybe padding?

see l.126. you may want to have a 

uint32_t padding;

explicitly to avoid surprise.

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
ppapi/api/private/pp_content_decryptor.idl:271: uint32_t data_size;
On 2013/10/11 01:30:53, DaleCurtis wrote:
> Is this necessary? The size is already attached to the PP_Resource.  I just
> followed what PP_DecryptedFrameInfo is doing...

You mean PP_DecryptedBufferInfo? See the comment on l.186.

[dmichael (off chromium), 2013-10-11 23:00:34]

FYI I'm expecting xhwang to do primary review since most of the audio stuff is
Greek to me.

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/26956002/diff/1/ppapi/api/private/pp_content_...
ppapi/api/private/pp_content_decryptor.idl:256: [assert_size(32)]
On 2013/10/11 01:30:53, DaleCurtis wrote:
> Not sure where the extra 4 bytes comes from, it counts out to 28 by my hand
> calculation... maybe padding?
PP_DecryptTrackingInfo has an int64_t, so it gets 8-byte aligned. I suspect
there's 4 bytes of padding between |data_size| and |tracking_info|.

[DaleCurtis, 2013-10-12 01:51:04]

https://codereview.chromium.org/26956002/diff/1/content/renderer/pepper/conte...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/26956002/diff/1/content/renderer/pepper/conte...
content/renderer/pepper/content_decryptor_delegate.cc:1042: int num_channel_ptrs
= 1;
On 2013/10/11 22:45:16, xhwang wrote:
> A brief comment why the default is 1? Interleaved bla bla..

Removed in favor of letting CopyFrom() handle sample format specifics.

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
File media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:326: // into the frame buffer
instead of a multistep serialization approach.
On 2013/10/11 22:45:16, xhwang wrote:
> Hmm, how much improvement does this approach buy us? It make the code a little
> bit harder to follow. For example, we return early at l.383 for case at l.328;
> and we have SetFrameBuffer on l.330 and l.386 which are pretty far away.
> 
> If the improvement is not trivial, can we wrap 355-377 to a helper function,
> call that function and return early right after l.344?

Hard to tell since we don't have pipeline tests for these CDM decoders.  I just
realized there's a better solution if we enable FFmpeg's ref counted frames
though.  We can just decode all the frames for a packet and stuff them in a
vector.  Once complete, we can dump them into the FrameBuffer object in one go. 
I'll write that up next week.

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:365: decoded_audio_size /
av_frame_->channels;
On 2013/10/11 22:45:16, xhwang wrote:
> shall we CHECK(decoded_audio_size % av_frame_->channels == 0) ? otherwise line
> 367 may overflow.

1. That wouldn't overflow since we'd always be rounding down, not up.
2. This is already DCHECK'd transitively (not the right word...) by line 303
since:

n % (ab) == 0 ==> n % a == 0 && n % b == 0.

My naive proof because I felt like it :)
1. n % (ab) == 0 :: n == cab for some c in Z.
2. Assume false, there's no x in Z such that n - xa == 0
3. n - xa != 0 :: cab - xa != 0
4. c in Z and b in Z ==> cb in Z
5. Let x == cb ==> xa - xa != 0
6. Repeat for b.

[DaleCurtis, 2013-10-14 19:04:45]

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
File media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:159:
serialized_audio_frames_.reserve(bytes_per_frame_ * samples_per_second_);
Removed since this allocates 172kb and most output frames max out around 8k. 
Letting vector manage the resize should be fine.

https://codereview.chromium.org/26956002/diff/1/media/cdm/ppapi/ffmpeg_cdm_au...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:326: // into the frame buffer
instead of a multistep serialization approach.
On 2013/10/12 01:51:05, DaleCurtis wrote:
> On 2013/10/11 22:45:16, xhwang wrote:
> > Hmm, how much improvement does this approach buy us? It make the code a
little
> > bit harder to follow. For example, we return early at l.383 for case at
l.328;
> > and we have SetFrameBuffer on l.330 and l.386 which are pretty far away.
> > 
> > If the improvement is not trivial, can we wrap 355-377 to a helper function,
> > call that function and return early right after l.344?
> 
> Hard to tell since we don't have pipeline tests for these CDM decoders.  I
just
> realized there's a better solution if we enable FFmpeg's ref counted frames
> though.  We can just decode all the frames for a packet and stuff them in a
> vector.  Once complete, we can dump them into the FrameBuffer object in one
go. 
> I'll write that up next week.

Actually this ended up more complicated since we need to preserve frames across
calls (and reuse them for the avcodec_decode_audio4 call) when there isn't
enough data for a frame.

I've reordered the code to be more legible.

[xhwang, 2013-10-14 20:06:41]

lgtm % nit

https://codereview.chromium.org/26956002/diff/9001/media/cdm/ppapi/ffmpeg_cdm...
File media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/26956002/diff/9001/media/cdm/ppapi/ffmpeg_cdm...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:106: const AVFrame* av_frame,
use const-ref for input?

[DaleCurtis, 2013-10-14 20:14:14]

https://codereview.chromium.org/26956002/diff/9001/media/cdm/ppapi/ffmpeg_cdm...
File media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/26956002/diff/9001/media/cdm/ppapi/ffmpeg_cdm...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:106: const AVFrame* av_frame,
On 2013/10/14 20:06:42, xhwang wrote:
> use const-ref for input?

Done.

[dmichael (off chromium), 2013-10-15 22:13:19]

lgtm

https://codereview.chromium.org/26956002/diff/20001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/26956002/diff/20001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.cc:1069: channel_ptrs[i] =
cur + i * size_per_channel;
optional suggestion: you don't actually need to multiply; you could add
size_per_channel each time. But what you have might be more readable and
performance is probably negligible at best.

https://codereview.chromium.org/26956002/diff/20001/media/cdm/ppapi/ffmpeg_cd...
File media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/26956002/diff/20001/media/cdm/ppapi/ffmpeg_cd...
media/cdm/ppapi/ffmpeg_cdm_audio_decoder.cc:410: }
Future suggestion: I don't think you've made this worse, but this function is
close to 200 lines and therefore a bit hard to follow. Might be nice to break it
up a little eventually.

At least one obvious chunk to make a function might be the body of the do/while
loop.

[commit-bot: I haz the power, 2013-10-22 00:39:50]

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

[commit-bot: I haz the power, 2013-10-22 01:04:00]

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

[commit-bot: I haz the power, 2013-10-22 01:36:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-22 01:45:02]

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

[commit-bot: I haz the power, 2013-10-22 02:28:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-22 17:49:12]

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

[commit-bot: I haz the power, 2013-10-22 23:19:44]

Change committed as 230250