Closed Bug 1318792 Opened 8 years ago Closed 7 years ago

[Widevine] Widevine encoded content not working in Firefox (works in Chrome)

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Version:
49 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: amit.patadia, Assigned: jay.harris)

References

Details

Attachments

(4 files, 3 obsolete files)

Encoded 4 second M4V file
126.66 KB, video/x-m4v
Details
Bug 1318792 - Adds support for sbgp and sgpd boxes in the traf box
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1318792 - Adds support for sbgp and sgpd boxes occuring in the sampletable
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1318792 - Adds a simple test for keys specified in the sgpd
59 bytes, text/x-review-board-request
jya
: review+
Details 
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
Group: firefox-core-security
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
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
Severity: normal → major
Priority: -- → P2
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.
Severity: major → normal
Priority: P2 → P1
Assignee: nobody → jharris
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.
Attachment #8830581 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8830582 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8830583 - Flags: review?(cpearce) → review?(jyavenard)
Given that you're changing MoofParser, I think that jya is a better reviewer.
Attachment #8830583 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8830581 - Attachment is obsolete: true
Attachment #8830581 - Flags: review?(jyavenard)
Attachment #8830582 - Attachment is obsolete: true
Attachment #8830582 - Flags: review?(jyavenard)
Attachment #8830583 - Attachment is obsolete: true
Attachment #8830583 - Flags: review?(jyavenard)
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
Attachment #8832207 - Flags: review?(jyavenard) → review+
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
Attachment #8832208 - Flags: review?(jyavenard) → review+
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...
Attachment #8832209 - Flags: review?(jyavenard) → review+
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
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d219333e5e3e
https://hg.mozilla.org/mozilla-central/rev/7a9bffdb4633
https://hg.mozilla.org/mozilla-central/rev/4d18b49a5b31
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
When can we expect mozilla54 to be released?
Depends on: 1387798
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: