Pass DecryptConfig parameters over IPC and use it in AVDA.

Pass DecryptConfig parameters over IPC and use it in AVDA.

This CL expands the BitstreamBuffer to pass the DecryptConfig
parameters (key ID, initialization vactor and the subsamples
lengths) to the Android Video Decoder Accelerator (AVDA)
and uses them by calling MediaCodec::QueueSecureInputBuffer()
if the frames are encrypted.

BUG=545091
Committed: https://crrev.com/47fb8a4962f80074e50608ff2d1cf1cf5bc7552d
Cr-Commit-Position: refs/heads/master@{#356438}

[Tima Vaisburd, 2015-10-21 23:04:28]

timav@chromium.org changed reviewers:
+ liberato@chromium.org, qinmin@chromium.org, xhwang@chromium.org

[Tima Vaisburd, 2015-10-21 23:04:28]

Please take a look

[xhwang, 2015-10-22 06:13:51]

looking good with a few nits

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/bitstream...
File media/base/bitstream_buffer.h (right):

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/bitstream...
media/base/bitstream_buffer.h:29: void SetDecryptParams(const DecryptConfig&
decrypt_config);
nit: maybe just SetDecryptConfig?

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/bitstream...
media/base/bitstream_buffer.h:60: std::vector<SubsampleEntry> subsamples_;  //
clear/cypher sizes
Can you just have a scoped_ptr<DecryptConfig> here?

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
File media/base/decrypt_config.cc (right):

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
media/base/decrypt_config.cc:44: os << std::setw(2) << static_cast<int>(ch);
Here and below, will base::HexEncode() make things easier?

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
File media/base/decrypt_config.h (right):

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
media/base/decrypt_config.h:78: inline std::ostream& operator<<(std::ostream&
os,
We don't like to overload operators, even for logging if not absolutely
necessary.

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Ove...

Can we just have a AsHumanReadableString() function that returns a string? It
won't be super efficient, but since it's only for logging in debug builds I
think it's fine...

[liberato (no reviews please), 2015-10-22 14:49:33]

lgtm

[Tima Vaisburd, 2015-10-22 17:15:45]

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/bitstream...
File media/base/bitstream_buffer.h (right):

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/bitstream...
media/base/bitstream_buffer.h:60: std::vector<SubsampleEntry> subsamples_;  //
clear/cypher sizes
On 2015/10/22 06:13:51, xhwang wrote:
> Can you just have a scoped_ptr<DecryptConfig> here?

Yes, I wanted that from the very beginning, but was stopped by l.62 comment. Do
you know why do we need the copy?

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
File media/base/decrypt_config.h (right):

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
media/base/decrypt_config.h:78: inline std::ostream& operator<<(std::ostream&
os,
On 2015/10/22 06:13:51, xhwang wrote:
> We don't like to overload operators, even for logging if not absolutely
> necessary.
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Ove...
> 
> Can we just have a AsHumanReadableString() function that returns a string? It
> won't be super efficient, but since it's only for logging in debug builds I
> think it's fine...

May I ask why? In my humble and maybe uninformed opinion operator<<() is
superior and it is also a well-known idiom. And it is for logging only, anyway?

[xhwang, 2015-10-22 18:11:25]

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/bitstream...
File media/base/bitstream_buffer.h (right):

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/bitstream...
media/base/bitstream_buffer.h:60: std::vector<SubsampleEntry> subsamples_;  //
clear/cypher sizes
On 2015/10/22 17:15:45, Tima Vaisburd wrote:
> On 2015/10/22 06:13:51, xhwang wrote:
> > Can you just have a scoped_ptr<DecryptConfig> here?
> 
> Yes, I wanted that from the very beginning, but was stopped by l.62 comment.
Do
> you know why do we need the copy?

ouch...

Maybe it's because of this that we allow copy and assign:

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_...

So this change looks good here. Could you please add a TODO about investigating
the possibility to disable copy and assign for this class? Thanks!

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
File media/base/decrypt_config.h (right):

https://chromiumcodereview.appspot.com/1422643002/diff/1/media/base/decrypt_c...
media/base/decrypt_config.h:78: inline std::ostream& operator<<(std::ostream&
os,
On 2015/10/22 17:15:45, Tima Vaisburd wrote:
> On 2015/10/22 06:13:51, xhwang wrote:
> > We don't like to overload operators, even for logging if not absolutely
> > necessary.
> > 
> >
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Ove...
> > 
> > Can we just have a AsHumanReadableString() function that returns a string?
It
> > won't be super efficient, but since it's only for logging in debug builds I
> > think it's fine...
> 
> May I ask why? In my humble and maybe uninformed opinion operator<<() is
> superior and it is also a well-known idiom. And it is for logging only,
anyway?

I searched around and do see some use of operator<< for logging. So this should
be fine. I think something like ToString() is used more often but I respect your
choice here :)

[Tima Vaisburd, 2015-10-22 21:57:26]

https://codereview.chromium.org/1422643002/diff/1/media/base/bitstream_buffer.h
File media/base/bitstream_buffer.h (right):

https://codereview.chromium.org/1422643002/diff/1/media/base/bitstream_buffer...
media/base/bitstream_buffer.h:29: void SetDecryptParams(const DecryptConfig&
decrypt_config);
On 2015/10/22 06:13:51, xhwang wrote:
> nit: maybe just SetDecryptConfig?

Done.

https://codereview.chromium.org/1422643002/diff/1/media/base/bitstream_buffer...
media/base/bitstream_buffer.h:60: std::vector<SubsampleEntry> subsamples_;  //
clear/cypher sizes
On 2015/10/22 18:11:25, xhwang wrote:
> On 2015/10/22 17:15:45, Tima Vaisburd wrote:
> > On 2015/10/22 06:13:51, xhwang wrote:
> > > Can you just have a scoped_ptr<DecryptConfig> here?
> > 
> > Yes, I wanted that from the very beginning, but was stopped by l.62 comment.
> Do
> > you know why do we need the copy?
> 
> ouch...
> 
> Maybe it's because of this that we allow copy and assign:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_...
> 
> So this change looks good here. Could you please add a TODO about
investigating
> the possibility to disable copy and assign for this class? Thanks!

Done.

https://codereview.chromium.org/1422643002/diff/1/media/base/decrypt_config.cc
File media/base/decrypt_config.cc (right):

https://codereview.chromium.org/1422643002/diff/1/media/base/decrypt_config.c...
media/base/decrypt_config.cc:44: os << std::setw(2) << static_cast<int>(ch);
On 2015/10/22 06:13:51, xhwang wrote:
> Here and below, will base::HexEncode() make things easier?

Done.

[xhwang, 2015-10-22 22:01:31]

Please s/in in/it in/ in the CL title.

Otherwise LGTM!

[Tima Vaisburd, 2015-10-22 22:09:03]

Description was changed from

==========
Pass DecryptConfig parameters over IPC and use in in ADVA.

This CL expands the BitstreamBuffer to pass the DecryptConfig
parameters (key ID, initialization vactor and the subsamples
lengths) to the Android Videi Decoder Accelerator (ADVA)
and uses them by calling MediaCodec::QueueSecureInputBuffer()
if the frames are encrypted.

BUG=545091
==========

to

==========
Pass DecryptConfig parameters over IPC and use it in ADVA.

This CL expands the BitstreamBuffer to pass the DecryptConfig
parameters (key ID, initialization vactor and the subsamples
lengths) to the Android Videi Decoder Accelerator (ADVA)
and uses them by calling MediaCodec::QueueSecureInputBuffer()
if the frames are encrypted.

BUG=545091
==========

[Tima Vaisburd, 2015-10-22 22:24:56]

timav@chromium.org changed reviewers:
+ sievers@chromium.org

[Tima Vaisburd, 2015-10-22 22:24:56]

+sievers@ as a content/ owner.

Daniel, could you, please, take a look?

[Tima Vaisburd, 2015-10-22 22:29:22]

timav@chromium.org changed reviewers:
+ dcheng@chromium.org

[Tima Vaisburd, 2015-10-22 22:29:23]

Adding dcheng@ as a content/common/gpu messages owner.

Daniel, could you please review this CL?

[watk, 2015-10-22 22:49:26]

watk@chromium.org changed reviewers:
+ watk@chromium.org

[watk, 2015-10-22 22:49:26]

I'm a drive by spellchecker.

Commit message has typos:
s/ADVA/AVDA/ twice
s/vactor/vector
s/videi/video

https://codereview.chromium.org/1422643002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1422643002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:316: // Normaly we
assume that the decoder makes at most one output frame for each
"Normally"

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
File media/base/decrypt_config.cc (right):

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
media/base/decrypt_config.cc:7: #include <iomanip>
Newline after this

[dcheng, 2015-10-23 00:44:47]

https://codereview.chromium.org/1422643002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1422643002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:213: status =
media_codec_->QueueSecureInputBuffer(
Why doesn't this function just take std::string for the key + IV and a
std::vector<media::SubsampleEntry> for the subsamples?

The other callers have to jump through these hoops too, so may as well make it
internal to the function.

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
File media/base/decrypt_config.cc (right):

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
media/base/decrypt_config.cc:7: #include <iomanip>
What is this for?

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
media/base/decrypt_config.cc:47: for (SubsampleEntry entry : subsamples_) {
This copies entry. const SubsampleEntry&

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
File media/base/decrypt_config.h (right):

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
media/base/decrypt_config.h:8: #include <ostream>
s/ostream/iosfwd

[Tima Vaisburd, 2015-10-23 18:35:23]

Description was changed from

==========
Pass DecryptConfig parameters over IPC and use it in ADVA.

This CL expands the BitstreamBuffer to pass the DecryptConfig
parameters (key ID, initialization vactor and the subsamples
lengths) to the Android Videi Decoder Accelerator (ADVA)
and uses them by calling MediaCodec::QueueSecureInputBuffer()
if the frames are encrypted.

BUG=545091
==========

to

==========
Pass DecryptConfig parameters over IPC and use it in AVDA.

This CL expands the BitstreamBuffer to pass the DecryptConfig
parameters (key ID, initialization vactor and the subsamples
lengths) to the Android Video Decoder Accelerator (AVDA)
and uses them by calling MediaCodec::QueueSecureInputBuffer()
if the frames are encrypted.

BUG=545091
==========

[Tima Vaisburd, 2015-10-23 18:51:41]

Spelling in the CL description is fixed.

https://codereview.chromium.org/1422643002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1422643002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:213: status =
media_codec_->QueueSecureInputBuffer(
On 2015/10/23 00:44:47, dcheng wrote:
> Why doesn't this function just take std::string for the key + IV and a
> std::vector<media::SubsampleEntry> for the subsamples?
> 
> The other callers have to jump through these hoops too, so may as well make it
> internal to the function.

I think because this function was designed to work with AccessUnit
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

I added an override that calls the existing one and TODO that only one should
stay after Spitzer pipeline prevails.

https://codereview.chromium.org/1422643002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:316: // Normaly we
assume that the decoder makes at most one output frame for each
On 2015/10/22 22:49:26, watk wrote:
> "Normally"

Done.

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
File media/base/decrypt_config.cc (right):

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
media/base/decrypt_config.cc:7: #include <iomanip>
On 2015/10/23 00:44:47, dcheng wrote:
> What is this for?

Missed during prior cleanup, removed.

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
media/base/decrypt_config.cc:47: for (SubsampleEntry entry : subsamples_) {
On 2015/10/23 00:44:47, dcheng wrote:
> This copies entry. const SubsampleEntry&

Done.

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
File media/base/decrypt_config.h (right):

https://codereview.chromium.org/1422643002/diff/40001/media/base/decrypt_conf...
media/base/decrypt_config.h:8: #include <ostream>
On 2015/10/23 00:44:47, dcheng wrote:
> s/ostream/iosfwd

Thank you, done.

[Tima Vaisburd, 2015-10-23 23:01:32]

Daniel, could you please take another look?

[dcheng, 2015-10-24 00:38:40]

lgtm

[Tima Vaisburd, 2015-10-24 00:45:05]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-10-24 00:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1422643002/#ps80001
(title: "Added missing license to bitstream_buffer.cc")

[commit-bot: I haz the power, 2015-10-24 00:45:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422643002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422643002/80001

[commit-bot: I haz the power, 2015-10-24 00:55:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-24 00:55:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[no sievers, 2015-10-26 17:52:28]

content lgtm

[Tima Vaisburd, 2015-10-27 01:20:35]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-10-27 01:20:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
liberato@chromium.org, xhwang@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1422643002/#ps120001
(title: "Made media::BitstreamBuffer exported")

[commit-bot: I haz the power, 2015-10-27 01:20:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422643002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422643002/120001

[commit-bot: I haz the power, 2015-10-27 05:22:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 05:22:27]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Tima Vaisburd, 2015-10-27 22:09:42]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-10-27 22:09:42]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
liberato@chromium.org, sievers@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1422643002/#ps80002
(title: "Removed MediaCodec.CodecException that was added only at level 21
(Android L)")

[commit-bot: I haz the power, 2015-10-27 22:11:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422643002/80002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422643002/80002

[commit-bot: I haz the power, 2015-10-27 23:45:05]

Committed patchset #9 (id:80002)

[commit-bot: I haz the power, 2015-10-27 23:46:58]

Patchset 9 (id:??) landed as
https://crrev.com/47fb8a4962f80074e50608ff2d1cf1cf5bc7552d
Cr-Commit-Position: refs/heads/master@{#356438}