User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20161019084923 Steps to reproduce: For the encrypted content http://shaka-player-demo.appspot.com/demo/?asset=http://storage.googleapis.com/wvtemp/alex/vz/enc/vz.mpd;license=http://104.198.7.31:8080/;play;logtoscreen;vv Actual results: Works in Chrome Does not work in Firefox (even if you click the Play button) Expected results: Should have worked in Firefox like Chrome

The encrypted content URI has changed to below since the older URI removed from http://shaka-player-demo.appspot.com. New URI: http://173.53.12.69/shaka-player/player.html plays in Chrome and does not play in Firefox

Unencrypted content URI: http://173.53.12.69/shaka-player/player_unencrypt.html plays in Chrome and Firefox.

That file doesn't play in Chrome 55 or Firefox for me on Windows.

@cpearce: The URI works for us on Chrome 55.0.2883.87 on Windows. Below is URL with recording showing that it works. http://173.53.12.69/shaka-player/bug_1318792.MOV Please note that the first 3-4 seconds is dark (part of the video) Can you let us know what is the error you get when you try to play?

The URL http://173.53.12.69/shaka-player/player.html plays in Chrome for me but not in Firefox.

It looks like this is happening because the video has a default key which should be overridden by the key in the sgpd box but isn't.

Indeed, as per "ISO Common Encryption ('cenc') Protection Scheme for ISO Base Media File Format Stream Format" [1]: "The encrypted block is a sample. Determining whether a sample is encrypted depends on the corresponding Track Encryption Box ('tenc') and the sample group with grouping type 'seig' (CencSampleEncryption group), if any, associated with the sample. The default encryption state of a sample is defined by the IsEncrypted flag in the associated track encryption box ('tenc'). This default state may be modified by the IsEncrypted flag in the SampleGroupDescriptionBox ('sgpd'), pointed to by an index in the SampleToGroupBox ('sbgp')." We don't parse sgpd and sbgp boxes. [1] https://www.w3.org/TR/eme-stream-mp4/#detect-encrypted-blocks

sgpd and sbgp boxes are specified in the ISO Base Media File Format spec, ISO/IEC 14496-12. http://l.web.umkc.edu/lizhu/teaching/2016sp.video-communication/ref/mp4.pdf The ISO/IEC 23001-7 specifies CencSampleEncryptionInformationVideoGroupEntry and CencSampleEncryptionInformationAudioGroupEntry boxes in section 6, "Encryption Parameters shared by groups of samples". See also Chromium's parser's implementation: https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.h?l=300 https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.cc?l=1216 We need our MP4 demuxer to parse these boxes, and build up a list of sample groups. We then need the demuxer to consult that list when outputting a MediaRawData so that if a sample is part of a group it gets the appropriate key.

Amit, I think I've got a fix but I'm trying to get some more test media, to confirm. How were your files encrypted?

@jharris: We use ‘cenc’ AES-CTR 128 bit scheme as per ISO/IEC 23001-7

Sorry, I mean what program did you use to encrypt them? I'm trying to create files some similar files for testing

@jharris: We use our own DASH packager for encryption. We can provide more files for your testing if you need. Let us know?

Some more files were testing would be great, would you be able to package this for me https://hg.mozilla.org/mozilla-central/raw-file/8ff550409e1d1f8b54f6f7f115545dbef857be0b/dom/media/test/short.mp4, we I can set up a test for it? Cheers.

Given that you're changing MoofParser, I think that jya is a better reviewer.

Comment on attachment 8832207 [details] Bug 1318792 - Adds support for sbgp and sgpd boxes in the traf box https://reviewboard.mozilla.org/r/108548/#review109706 ::: dom/media/fmp4/MP4Demuxer.cpp:364 (Diff revision 1) > if (sample->mCrypto.mValid) { > nsAutoPtr<MediaRawDataWriter> writer(sample->CreateWriter()); > writer->mCrypto.mMode = mInfo->mCrypto.mMode; > writer->mCrypto.mIVSize = mInfo->mCrypto.mIVSize; > + > + if (writer->mCrypto.mKeyId.Length() == 0) { Please add a comment on what you're doing here and why. (e.g. use default key if none is defined for sample) ::: media/libstagefright/binding/Index.cpp:140 (Diff revision 1) > } > ByteReader reader(cenc); > writer->mCrypto.mValid = true; > writer->mCrypto.mIVSize = ivSize; > > + nsTArray<uint8_t> keyIdArray; why not work directly with write->mCrypto.mKeyId, why the use of the intermediary variable? if you do want to use an intermediary variable, use Move(keyIdArray) ::: media/libstagefright/binding/Index.cpp:184 (Diff revision 1) > + Moof* currentMoof = &moofs[mCurrentMoof]; > + SampleToGroupEntry* sampleToGroupEntry = nullptr; > + > + uint32_t seen = 0; > + > + for (uint32_t i = 0; i < currentMoof->mSampleToGroupEntries.Length(); ++i) { for (SampleToGroupEntry& entry : currentMoof->mSampleToGroupEntries) { .... sampleToGroupEntry = & entry; ::: media/libstagefright/binding/Index.cpp:214 (Diff revision 1) > + group_index -= SampleToGroupEntry::kFragmentGroupDescriptionIndexBase; > + } > + > + // The group_index is one indexed > + return group_index > currentMoof->mSampleEncryptionInfoEntries.Length() > + ? nullptr FWIW, ?: should be indented with the variable above code style should be fixed at some stage. brownie points if this is done in another patch. ::: media/libstagefright/binding/MoofParser.cpp:923 (Diff revision 1) > } > } > mValid = true; > } > > +Sbgp::Sbgp(Box & aBox) Box& aBox ::: media/libstagefright/binding/MoofParser.cpp:960 (Diff revision 1) > + LOG(Sbgp, "Incomplete Box (have:%lld, need:%lld). Failed to read entries", > + (uint64_t)reader->Remaining(), (uint64_t)need); > + return; > + } > + > + for (uint32_t i = 0; i < count; ++i) { i++ like everything else ::: media/libstagefright/binding/MoofParser.cpp:967 (Diff revision 1) > + uint32_t groupDescriptionIndex = reader->ReadU32(); > + > + SampleToGroupEntry entry(sampleCount, groupDescriptionIndex); > + mEntries.AppendElement(entry); > + } > +} should set mValid to true when things are valid. And test if box is valid later. ::: media/libstagefright/binding/MoofParser.cpp:1003 (Diff revision 1) > + > + uint32_t count = reader->ReadU32(); > + > + // Make sure we have sufficient remaining bytes to read the entries. > + need = count > + * (sizeof(uint32_t) what kind of indentation is this ? first * should be aligned with count second * should be aligned with [s]izeof not the ( fwiw, clang-format gives me : need = count * (sizeof(uint32_t) * (version == 1 && defaultLength == 0 ? 2 : 1) + kKeyIdSize * sizeof(uint8_t)); ::: media/libstagefright/binding/MoofParser.cpp:1015 (Diff revision 1) > + return; > + } > + for (uint32_t i = 0; i < count; ++i) { > + if (version == 1 && defaultLength == 0) { > + uint32_t descriptionLength = reader->ReadU32(); > + MOZ_ASSERT(descriptionLength >= entrySize); maybe we should ignore entry instead, to avoid being beaten with corrupted data ::: media/libstagefright/binding/MoofParser.cpp:1021 (Diff revision 1) > + } > + > + CencSampleEncryptionInfoEntry entry(reader); > + mEntries.AppendElement(entry); > + } > +} same deal with mValid ::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h:173 (Diff revision 1) > + static const uint32_t kFragmentGroupDescriptionIndexBase = 0x10000; > + > + SampleToGroupEntry(uint32_t aSampleCount, uint32_t aGroupDescriptionIndex) > + : mSampleCount(aSampleCount) > + , mGroupDescriptionIndex(aGroupDescriptionIndex) > + {} { } on two lines

Comment on attachment 8832208 [details] Bug 1318792 - Adds support for sbgp and sgpd boxes occuring in the sampletable https://reviewboard.mozilla.org/r/108550/#review109716 ::: media/libstagefright/binding/Index.cpp:192 (Diff revision 1) > + ? &currentMoof->mFragmentSampleToGroupEntries > + : &mIndex->mMoofParser->mTrackSampleToGroupEntries; > + > uint32_t seen = 0; > > - for (uint32_t i = 0; i < currentMoof->mSampleToGroupEntries.Length(); ++i) { > + for (uint32_t i = 0; i < sampleToGroupEntries->Length(); ++i) { as in P1 for (SampleToGroupEntry& entry : sampleToGroupEntries) { .... ::: media/libstagefright/binding/Index.cpp:218 (Diff revision 1) > + // groupDescriptionIndex 0. > if (!sampleToGroupEntry || sampleToGroupEntry->mGroupDescriptionIndex == 0) { > return nullptr; > } > > - uint32_t group_index = sampleToGroupEntry->mGroupDescriptionIndex; > + nsTArray<CencSampleEncryptionInfoEntry>* entries = &mIndex->mMoofParser->mTrackSampleEncryptionInfoEntries; 80 columns wrapping ::: media/libstagefright/binding/MoofParser.cpp:322 (Diff revision 1) > for (Box box = aBox.FirstChild(); box.IsAvailable(); box = box.Next()) { > if (box.IsType("stsd")) { > ParseStsd(box); > + } else if (box.IsType("sgpd")) { > + Sgpd sgpd(box); > + if (sgpd.mGroupingType == "seig") { check that box is valid too there

Comment on attachment 8832209 [details] Bug 1318792 - Adds a simple test for keys specified in the sgpd https://reviewboard.mozilla.org/r/108552/#review109720 ::: dom/media/test/test_eme_sample_groups_playback.html:126 (Diff revision 1) > + .then(() => { > + var ms = new MediaSource(); > + video.src = URL.createObjectURL(ms); > + > + once(ms, "sourceopen", () => { > + Promise.all(test.track.fragments.map(fragment => DownloadMedia(fragment, test.track.type, ms))) wrong indentation...

Comment on attachment 8832209 [details] Bug 1318792 - Adds a simple test for keys specified in the sgpd https://reviewboard.mozilla.org/r/108552/#review109738 ::: dom/media/test/test_eme_sample_groups_playback.html:126 (Diff revision 1) > + .then(() => { > + var ms = new MediaSource(); > + video.src = URL.createObjectURL(ms); > + > + once(ms, "sourceopen", () => { > + Promise.all(test.track.fragments.map(fragment => DownloadMedia(fragment, test.track.type, ms))) Fixed, I think (was this just that Promise.all and following thens weren't indented?

Comment on attachment 8832207 [details] Bug 1318792 - Adds support for sbgp and sgpd boxes in the traf box https://reviewboard.mozilla.org/r/108548/#review109832 ::: media/libstagefright/binding/MoofParser.cpp:925 (Diff revisions 1 - 3) > mValid = true; > } > > -Sbgp::Sbgp(Box & aBox) > +Sbgp::Sbgp(Box& aBox) > { > + mValid = true; mValid is initialised as false upon construction. should simply set it to true right at the end of the constructor. so you don't need to modify its value prior any abnormal return ::: media/libstagefright/binding/MoofParser.cpp:1040 (Diff revisions 1 - 3) > + mValid = false; > + return; > + } > mEntries.AppendElement(entry); > } > } just set mValid = true here ::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h:193 (Diff revisions 1 - 3) > > struct CencSampleEncryptionInfoEntry final > { > public: > - explicit CencSampleEncryptionInfoEntry(BoxReader& aReader); > + CencSampleEncryptionInfoEntry() > + { } { } on one line after the prototyping or if not on the same line, on two diffent line: { }

Comment on attachment 8832208 [details] Bug 1318792 - Adds support for sbgp and sgpd boxes occuring in the sampletable https://reviewboard.mozilla.org/r/108550/#review109834 ::: media/libstagefright/binding/MoofParser.cpp:938 (Diff revisions 1 - 3) > mValid = true; > } > > -Sbgp::Sbgp(Box & aBox) > +Sbgp::Sbgp(Box& aBox) > { > + mValid = true; same comment for mValid ; only set it to true once you've completed the code. no point setting it up whenever you return early

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d219333e5e3e Adds support for sbgp and sgpd boxes in the traf box r=jya https://hg.mozilla.org/integration/autoland/rev/7a9bffdb4633 Adds support for sbgp and sgpd boxes occuring in the sampletable r=jya https://hg.mozilla.org/integration/autoland/rev/4d18b49a5b31 Adds a simple test for keys specified in the sgpd r=jya

https://hg.mozilla.org/mozilla-central/rev/d219333e5e3e https://hg.mozilla.org/mozilla-central/rev/7a9bffdb4633 https://hg.mozilla.org/mozilla-central/rev/4d18b49a5b31

When can we expect mozilla54 to be released?