1118597 - MoofParser broken for some CENC files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Bug 1110608 broke parsing of some fragmented CENC files. I'm disabling MoofParser for encrypted files in bug 1118593, but we still want to use MoofParser for encrypted files, since it fixes other issues.

So, this bug is to track fixing MoofParser for encrypted files.

STR:
1. Get gmp-clearkey from https://github.com/cpearce/gmp-clearkey
2. Load http://people.mozilla.org/~cpearce/eme/
3. Observe crash; the GMPDecryptor asserts that it expected the sample to have valid crypto data, but it doesn't.

In debugging this, it looked like the auxInfoType of the saiz box was 'sinf', and so we didn't setup crypto data. We set the default auxInfoType of the saiz box to "sinf", but ISO/IEC 23001-7:2012(E) says that for CENC files a auxInfoType of 'cenc' should be assumed. I still crashed when I changed the default, so that's not the only problem.

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1118597.patch

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1118597-enable-moofparser-eme.patch

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8551531 [details] [diff] [review]
1118597.patch

Review of attachment 8551531 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a fan a cascade of parseXXX that you have now.

The existing style was to have constructor taking a Box and being "self-aware".

::: media/libstagefright/binding/Box.cpp
@@ +27,5 @@
> +    return 78;
> +  } else if (aType == AtomType("stsd")) {
> +    return FULLBOX_OFFSET + 4;
> +  }
> +

please document where those values come from

@@ +79,4 @@
>    }
>  
> +  mType = BigEndian::readUint32(&header[4]);
> +  mChildOffset = mBodyOffset + BoxOffset(mType);

probably should check that child offset is still within the box range and abort if not.

::: media/libstagefright/binding/Index.cpp
@@ +108,5 @@
>    }
>  
>    if (!s->mCencRange.IsNull()) {
> +    MoofParser* parser = mIndex->mMoofParser.get();
> +    MOZ_ASSERT(parser && parser->mSinf.IsValid());

can't we just exit and stop there. File won't play but at least it won't crash

@@ +116,4 @@
>      // The size comes from an 8 bit field
>      nsAutoTArray<uint8_t, 256> cenc;
>      cenc.SetLength(s->mCencRange.Length());
>      if (!mIndex->mSource->ReadAt(s->mCencRange.mStart, &cenc[0], cenc.Length(),

it's a carry-on from earlier, but I dislike the use of &cenc[0] ; this will assert if the array is empty.
cenc.Elements() won't

@@ +122,5 @@
>      }
>      ByteReader reader(cenc);
>      sample->crypto.valid = true;
> +    sample->crypto.iv_size = ivSize;
> +    reader.ReadArray(sample->crypto.iv, ivSize);

test return value of ReadArray

::: media/libstagefright/binding/MoofParser.cpp
@@ +203,5 @@
> +    // Some MP4 files have been found to have multiple sinf boxes in the same
> +    // enc* box. This does not match spec anyway, so just choose the first
> +    // one that parses properly.
> +    if (box.IsType("sinf") && !mSinf.IsValid()) {
> +      ParseSinf(box);

can't you simply break the loop instead? no need to loop when we're not going to do anything later

@@ +209,5 @@
> +  }
> +}
> +
> +static void
> +ParseSchm(Box& aBox, Sinf& aSinf)

a bit inconsistent with the rest of the code, make them private member of MoofReader

@@ +212,5 @@
> +static void
> +ParseSchm(Box& aBox, Sinf& aSinf)
> +{
> +  BoxReader reader(aBox);
> +  uint32_t flags = reader->ReadU32(); // ignore

test that we have 8 bytes available in BoxReader

@@ +220,5 @@
> +  reader->DiscardRemaining();
> +}
> +
> +static void
> +ParseTenc(Box& aBox, Sinf& aSinf)

and here

@@ +226,5 @@
> +  BoxReader reader(aBox);
> +  uint32_t flags = reader->ReadU32(); // ignore
> +
> +  uint32_t isEncrypted = reader->ReadU24();
> +  aSinf.mDefaultIVSize = reader->ReadU8();

test that we have 4 + 3 + 1 + 16...

@@ +229,5 @@
> +  uint32_t isEncrypted = reader->ReadU24();
> +  aSinf.mDefaultIVSize = reader->ReadU8();
> +
> +  memcpy(aSinf.mDefaultKeyID, reader->Read(16), 16);
> +}

The construction of the Sinf should be done in a Sinf constructor that takes a Box& argument (in line with the rest of the code)

@@ +232,5 @@
> +  memcpy(aSinf.mDefaultKeyID, reader->Read(16), 16);
> +}
> +
> +static void
> +ParseSchi(Box& aBox, Sinf& aSinf)

and here

::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h
@@ +119,5 @@
> +
> +  uint8_t mDefaultIVSize;
> +  AtomType mDefaultEncryptionType;
> +  uint8_t mDefaultKeyID[16];
> +};

Build the Sinf in a Sinf(Box& aBox) constructor, similar to all other atoms.

@@ +133,5 @@
>  
>  class Saiz
>  {
>  public:
> +  explicit Saiz(Box& aBox, Sinf& aSinf);

if you takes two arguments, you don't need explicit.

However, I think it should take the info type as argument rather than the Sinf; so we untangle what is starting to become messy. Keep it abstracted so a Saiz and Saio box don't need to be aware of the internal implementation of the Sinf

@@ +143,5 @@
>  
>  class Saio
>  {
>  public:
> +  explicit Saio(Box& aBox, Sinf& aSinf);

same as above, take the encryption type as argument

Comment 4

[Jean-Yves Avenard [:jya]]

> +    if (box.IsType("sinf") && !mSinf.IsValid()) {
> +      ParseSinf(box);

I meant:
    if (box.IsType("sinf")) {
       ParseSinf(box);
       if (mSinf.IsValid()) {
         break;
       }

though ParseSinf should return a boolean that it has succeeded.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1118597.patch v2

Addressed review comments and IRC comments.

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1118597.patch v2.1

Actually address review comments and IRC comments.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8551614 [details] [diff] [review]
1118597.patch v2.1

Review of attachment 8551614 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed

::: media/libstagefright/binding/Index.cpp
@@ +136,5 @@
>        uint16_t count = reader.ReadU16();
>        for (size_t i = 0; i < count; i++) {
>          sample->crypto.plain_sizes.AppendElement(reader.ReadU16());
>          sample->crypto.encrypted_sizes.AppendElement(reader.ReadU32());
>        }

existing problem, but as this patch is a fix, should test the cenc contains the right amount of bytes and is well formed (otherwise this will assert).

::: media/libstagefright/binding/MoofParser.cpp
@@ +173,5 @@
> +  for (Box box = aBox.FirstChild(); box.IsAvailable(); box = box.Next()) {
> +    // Some MP4 files have been found to have multiple sinf boxes in the same
> +    // enc* box. This does not match spec anyway, so just choose the first
> +    // one that parses properly.
> +    if (box.IsType("sinf") && !mSinf.IsValid()) {

mSinf.IsValid() isn't required as as soon it is valid we exit the loop

@@ +272,5 @@
>          tfdt = Tfdt(box);
>        } else if (box.IsType("trun")) {
>          ParseTrun(box, tfhd, tfdt, aMdhd, aEdts);
>        } else if (box.IsType("saiz")) {
> +        mSaizs.AppendElement(Saiz(box, aSinf));

mSaizs.AppendElement(Saiz(box, aSinf.GetEncryptionType())

::: media/libstagefright/binding/SinfParser.cpp
@@ +25,5 @@
> +      ParseSchi(box);
> +    }
> +  }
> +
> +  mSinf.CheckValid();

not required if you simply override IsValid()

@@ +37,5 @@
> +  if (reader->Remaining() < 8) {
> +    return;
> +  }
> +
> +  uint32_t flags = reader->ReadU32(); // ignore

warning unused variables...

@@ +62,5 @@
> +  if (reader->Remaining() < 24) {
> +    return;
> +  }
> +
> +  uint32_t flags = reader->ReadU32(); // ignore

warning unused variables...

::: media/libstagefright/binding/include/mp4_demuxer/Box.h
@@ +58,4 @@
>    uint64_t mChildOffset;
>    AtomType mType;
>    const Box* mParent;
>  };

Should probably have a method to check that Box is valid, as right now any errors reading the box and setting its member won't be propagated.

This can be done in a another patch/bug #

::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h
@@ +136,5 @@
>  
>  class Saiz : public Atom
>  {
>  public:
> +  Saiz(Box& aBox, Sinf& aSinf);

Saiz(Box& aBox, const AtomType& aEncryptionType)
Due to the implementation of AtomType, probably can not bother with const ref

@@ +146,5 @@
>  
>  class Saio : public Atom
>  {
>  public:
> +  Saio(Box& aBox, Sinf& aSinf);

Saio(Box& aBox, const AtomType& aEncryptionType)

::: media/libstagefright/binding/include/mp4_demuxer/SinfParser.h
@@ +21,5 @@
> +    , mDefaultIVSize(0)
> +  {}
> +  explicit Sinf(Box& aBox);
> +
> +  void CheckValid() { mValid = !!mDefaultIVSize && !!mDefaultEncryptionType; }

Use existing IsValid() API, no need to set mValid.
mValid is only of use if you don't have a IsValid() and you then use the default implementation

@@ +24,5 @@
> +
> +  void CheckValid() { mValid = !!mDefaultIVSize && !!mDefaultEncryptionType; }
> +
> +  uint8_t mDefaultIVSize;
> +  AtomType mDefaultEncryptionType;

something like ..
AtomType GetEncryptionType()
{
  return IsValid() ? mDefaultEncryptionType : AtomType()
}

Comment 8

[Jean-Yves Avenard [:jya]]

 0:03.87 Warning: -Wreorder in /Users/jyavenard/Work/Mozilla/mozilla-central/media/libstagefright/binding/include/mp4_demuxer/SinfParser.h: field 'mDefaultEncryptionType' will be initialized after field 'mDefaultIVSize'
 0:03.87 /Users/jyavenard/Work/Mozilla/mozilla-central/media/libstagefright/binding/include/mp4_demuxer/SinfParser.h:20:7: warning: field 'mDefaultEncryptionType' will be initialized after field 'mDefaultIVSize' [-Wreorder]
 0:03.87     : mDefaultEncryptionType()
 0:03.87       ^
 0:03.87 1 warning generated.

Something weird on my box:
1:05.58 ../../../dist/include/mp4_demuxer/MoofParser.h:8:10: fatal error: 'mp4_demuxer/Atom.h' file not found
 1:05.58 #include "mp4_demuxer/Atom.h"

jyapro:mozilla-central jyavenard$ ls -l media/libstagefright/binding/include/mp4_demuxer/Atom*
-rw-r--r--+ 1 jyavenard  staff  425 20 Jan 17:39 media/libstagefright/binding/include/mp4_demuxer/Atom.h
-rw-r--r--+ 1 jyavenard  staff  833 20 Jan 17:39 media/libstagefright/binding/include/mp4_demuxer/AtomType.h

it's there!

Comment 9

[Jean-Yves Avenard [:jya]]

Oh..

It's missing the required entry in moz.build...

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8551614 [details] [diff] [review]
1118597.patch v2.1

Review of attachment 8551614 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, on second look

Where are 

MoofParser::ParseMinf
MoofParser::ParseStbl
MoofParset::ParseStsd
MoofParser::ParseSinf

?

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1118597.patch v3

Re-added missing stuff, whipped hg into line, addressed comments.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> something like ..
> AtomType GetEncryptionType()
> {
>   return IsValid() ? mDefaultEncryptionType : AtomType()
> }

This shouldn't really matter. We only set mSinf if we have a valid Sinf, otherwise mDefaultEncryptionType is 0; further we don't bother with getters elsewhere in MoofParser.

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8552063 [details] [diff] [review]
1118597.patch v3

Review of attachment 8552063 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with some nits.

::: media/libstagefright/binding/Index.cpp
@@ +131,5 @@
> +    if (!reader.ReadArray(sample->crypto.iv, ivSize)) {
> +      return nullptr;
> +    }
> +
> +    if (reader.CanReadType<uint16_t>()) {

CanRead16 ?

::: media/libstagefright/binding/SinfParser.cpp
@@ +14,5 @@
> +  SinfParser parser(aBox);
> +  if (parser.GetSinf().IsValid()) {
> +    *this = parser.GetSinf();
> +  }
> +}

as we can use mSinf.mDefaultEncryptionType without mSinf being valid, it needs to be initialized to 0 regardless.

::: media/libstagefright/binding/include/mp4_demuxer/SinfParser.h
@@ +17,5 @@
> +{
> +public:
> +  Sinf()
> +    : mDefaultEncryptionType()
> +    , mDefaultIVSize(0)

mDefaultIVSize must be initialized before mDefaultEncryptionType (cause a compilation warning otherwise)

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f3a324e645d
https://hg.mozilla.org/mozilla-central/rev/06648aa9a5c4

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8552063 [details] [diff] [review]
1118597.patch v3

Requesting 37 uplift of just this patch.

Approval Request Comment
[Feature/regressing bug #]: MSE.
[User impact if declined]: Less consistent testing, harder to port critical fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c some time ago.
[Risks and why]: This is an EME patch, not an MSE one, but it does no harm and simplifies porting important MSE. Risk is minimal.
[String/UUID change made/needed]: None.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8552063 [details] [diff] [review]
1118597.patch v3

I don't have much concern about this one as it has been on m-c for some time already. Let's make things easier on ourselves for MSE in 37. Aurora+

Comment 17

[Ralph Giles (:rillian)]

Both patches pushed to aurora.
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=f24f0b0532f9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Something in this push caused linux32 debug bustage. Nobody was around to help, so I had to back it out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/77d5e3a30435

https://treeherder.mozilla.org/logviewer.html#?job_id=612948&repo=mozilla-aurora

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Also w-p-t permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=614652&repo=mozilla-aurora

Comment 20

[Ralph Giles (:rillian)]

Trying again. I added an include to work around the (linux32 debug only!) build failure in Sinfparser.cpp.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed269dd0a199

Comment 21

[Syd Polk :sydpolk]

I don't see a crash when following these steps with Aurora, but I don't see any media either.