Update EME MediaKeyNeededEvent.initData for ISO BMFF to match current spec.

Update EME MediaKeyNeededEvent.initData for ISO BMFF to match current spec.

This change causes needkey events to be fired when ever a 'moov' or 'moof' box is encountered which contains PSSH boxes. The value of initData is the concatenation of all PSSH boxes in that top-level element.

BUG=132351
TEST=Manual verification using nonredistributable sample media


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158176

[strobe_, 2012-08-05 00:40:25]

Here's an update to follow the current thinking on
https://www.w3.org/Bugs/Public/show_bug.cgi?id=17673 .

This change will break most existing prototype apps:

- Most files we've seen put a 'pssh' on every 'moov/moof', so we'll get a lot of
repeated events / infinite loops,

- Most apps have been assuming initData is a Clear Key/CENC key ID, and now it
is not, and

- Most apps use the the same value for the initData arg on addKey() as was
received on MediaKeyNeededEvent.initData, rather than passing in a raw key ID.

Due to these issues, it may be wise to defer this CL until a provisional Clear
Key PSSH format is defined.

The CENC encryptor's almost feature-complete; when it's done, I'll add tests and
sample media that we can redistribute.

[ddorwin, 2012-09-21 04:04:28]

This seems fine overall unless we're going to see many more events.

On 2012/08/05 00:40:25, strobe wrote:
> Here's an update to follow the current thinking on
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=17673 .
> 
> This change will break most existing prototype apps:
> 
> - Most files we've seen put a 'pssh' on every 'moov/moof', so we'll get a lot
of
> repeated events / infinite loops,

Why infinite loops? Are you going to get more events than previously?
> 
> - Most apps have been assuming initData is a Clear Key/CENC key ID, and now it
> is not, and
Ideally, the apps should not need to change much. There should be a Clear Key
PSSH format: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17682.
> 
> - Most apps use the the same value for the initData arg on addKey() as was
> received on MediaKeyNeededEvent.initData, rather than passing in a raw key ID.
If not doing ClearKey, initData is not really necessary. Until we have a key
ID-key association schemed
(https://www.w3.org/Bugs/Public/show_bug.cgi?id=17682) and Chrome is updated,
the initData parameter of addKey needs to be the keyID associated with the key.

> 
> Due to these issues, it may be wise to defer this CL until a provisional Clear
> Key PSSH format is defined.
> 
> The CENC encryptor's almost feature-complete; when it's done, I'll add tests
and
> sample media that we can redistribute.

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_definiti...
File media/mp4/box_definitions.cc (right):

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_definiti...
media/mp4/box_definitions.cc:37: raw_box.clear();
Should it ever not be empty?

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_definiti...
media/mp4/box_definitions.cc:38: raw_box.insert(raw_box.end(),
Odd to use end() when you just cleared it. Can you use assign() instead?

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_reader.h
File media/mp4/box_reader.h (right):

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_reader.h...
media/mp4/box_reader.h:57: const uint8* data() { return buf_; }
Can this be a const function? The two below should be.

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
File media/mp4/mp4_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
media/mp4/mp4_stream_parser.cc:277: bool MP4StreamParser::EmitKeyNeeded(
The name implies one will always be emitted, as used to be the case. Add
IfNecessary or IfEncrypted or IfPotentiallyEncrypted?

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
media/mp4/mp4_stream_parser.cc:283: return true;
You should probably define the return value as "sent successfully if necessary
or not necessary" or something like that. It's really that false means an error
occurred. Without clarification, I would not expect true from emit() if it
didn't emit.

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
media/mp4/mp4_stream_parser.cc:296: return need_key_cb_.Run(init_data.Pass(),
total_size);
Future CL:
We should probably change the name of this function and callback. This callback
means "I found some initData". It's TBD how to dedupe initData, especially since
they don't map directly to key IDs for ISO.

[strobe_, 2012-09-21 18:29:46]

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_definiti...
File media/mp4/box_definitions.cc (right):

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_definiti...
media/mp4/box_definitions.cc:37: raw_box.clear();
On 2012/09/21 04:04:28, ddorwin wrote:
> Should it ever not be empty?

Changed to DCHECK(empty())

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_definiti...
media/mp4/box_definitions.cc:38: raw_box.insert(raw_box.end(),
On 2012/09/21 04:04:28, ddorwin wrote:
> Odd to use end() when you just cleared it. Can you use assign() instead?

Done.

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_reader.h
File media/mp4/box_reader.h (right):

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/box_reader.h...
media/mp4/box_reader.h:57: const uint8* data() { return buf_; }
On 2012/09/21 04:04:28, ddorwin wrote:
> Can this be a const function? The two below should be.

Done.

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
File media/mp4/mp4_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
media/mp4/mp4_stream_parser.cc:277: bool MP4StreamParser::EmitKeyNeeded(
On 2012/09/21 04:04:28, ddorwin wrote:
> The name implies one will always be emitted, as used to be the case. Add
> IfNecessary or IfEncrypted or IfPotentiallyEncrypted?

Done.

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
media/mp4/mp4_stream_parser.cc:283: return true;
On 2012/09/21 04:04:28, ddorwin wrote:
> You should probably define the return value as "sent successfully if necessary
> or not necessary" or something like that. It's really that false means an
error
> occurred. Without clarification, I would not expect true from emit() if it
> didn't emit.

Done.

https://chromiumcodereview.appspot.com/10837116/diff/1/media/mp4/mp4_stream_p...
media/mp4/mp4_stream_parser.cc:296: return need_key_cb_.Run(init_data.Pass(),
total_size);
On 2012/09/21 04:04:28, ddorwin wrote:
> Future CL:
> We should probably change the name of this function and callback. This
callback
> means "I found some initData". It's TBD how to dedupe initData, especially
since
> they don't map directly to key IDs for ISO.

Agreed.

[strobe_, 2012-09-21 18:41:36]

On 2012/09/21 04:04:28, ddorwin wrote:
> This seems fine overall unless we're going to see many more events.
> 
> On 2012/08/05 00:40:25, strobe wrote:
> > Here's an update to follow the current thinking on
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17673 .
> > 
> > This change will break most existing prototype apps:
> > 
> > - Most files we've seen put a 'pssh' on every 'moov/moof', so we'll get a
lot
> of
> > repeated events / infinite loops,
> 
> Why infinite loops? Are you going to get more events than previously?

Old files (those derived from PIFF) tended to have 'pssh' at every 'moof', and
some test pages had naïve logic like 'call "addkey" as soon as you get
"needkey"' combined with 'start the append callback whenever you get
"keyadded"'. So this is just a matter of really really naïve Clear Key test
rigs.

Because MSE still invokes 14496-12:2008, there's no way to make use of key
rotation without either sending all keys up front or sending new 'moov' atoms
every so often (which is what YT will do). As a result, 'pssh'-per-'moof' is
pretty useless, and is unlikely to be seen in the wild. This doesn't solve the
dedupe problem, but it does limit its impact for now.

(Other comments covered in separate PSSH proposal thread)

[ddorwin, 2012-09-22 03:25:05]

lgtm % comment

https://chromiumcodereview.appspot.com/10837116/diff/5001/media/mp4/box_reader.h
File media/mp4/box_reader.h (right):

https://chromiumcodereview.appspot.com/10837116/diff/5001/media/mp4/box_reade...
media/mp4/box_reader.h:57: const uint8* data() const { return buf_; };
You don't need the last semicolon.

[strobe_, 2012-09-22 04:05:33]

https://chromiumcodereview.appspot.com/10837116/diff/5001/media/mp4/box_reader.h
File media/mp4/box_reader.h (right):

https://chromiumcodereview.appspot.com/10837116/diff/5001/media/mp4/box_reade...
media/mp4/box_reader.h:57: const uint8* data() const { return buf_; };
On 2012/09/22 03:25:06, ddorwin wrote:
> You don't need the last semicolon.

I just wanted to be sure that my statement was really, really terminated.

[commit-bot: I haz the power, 2012-09-22 04:05:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/strobe@google.com/10837116/12001

[commit-bot: I haz the power, 2012-09-22 06:20:50]

Change committed as 158176