1066427 - Add MP4 Common Encryption support to MP4 demuxer for non-fragmented MP4 files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Attachment: non-fragmented-eme.patch

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8488409 [details] [diff] [review]
non-fragmented-eme.patch

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

Lots of stuff that doesn't follow local style. Especially indentation and aParameter.

::: media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp
@@ +446,5 @@
>  
>      return OK;
>  }
>  
> +static uint32_t

I think this should be status_t.

@@ +448,5 @@
>  }
>  
> +static uint32_t
> +ValidateCencBoxHeader(DataSource* aDataSource, off64_t& aDataOffset,
> +                      uint8_t* aOutVersion, uint32_t* aOutAuxType)

Should use local style here, ugly as it is. You should probably be using a pointer to aDataOffset to make it mildly clearer at the call site that it gets modified.

Suggestion: use a sp<DataSource>& instead of having the get() below. Same efficiency, a little more clarity.

@@ +453,5 @@
> +{
> +    *aOutAuxType = 0;
> +
> +    if (aDataSource->readAt(aDataOffset++, aOutVersion,
> +                            sizeof(*aOutVersion)) < sizeof(*aOutVersion)) {

Suggestion: I'm not convinced that sizeof(*aOutVersion) adds any value over writing 1, given that aDataOffset++ essentially hardcodes the size anyway. Elsewhere it is just written as a constant. Lipstick on a pig.

@@ +483,5 @@
> +
> +status_t
> +SampleTable::setSampleAuxiliaryInformationSizeParams(off64_t aDataOffset,
> +                                                     size_t aDataSize,
> +                                                     uint32_t aDrmScheme)

Local style.

@@ +490,5 @@
> +
> +    uint8_t version;
> +    uint32_t auxType;
> +    status_t err = ValidateCencBoxHeader(mDataSource.get(), aDataOffset,
> +                                         &version, &auxType);

Local style.

@@ +597,5 @@
> +SampleTable::parseSampleCencInfo()
> +{
> +    if (!mCencDefaultSize && !mCencInfoCount || mCencOffsets.isEmpty()) {
> +        // We don't have all the cenc information we need yet. Quietly fail and
> +        // hope we get the data we need later in the track header.

Suggestion: It may be worth adding some logging here.

@@ +617,5 @@
> +        uint8_t size = mCencDefaultSize ? mCencDefaultSize : mCencSizes[i];
> +        uint64_t offset = mCencOffsets.size() == 1 ? nextOffset : mCencOffsets[i];
> +        nextOffset = offset + size;
> +
> +        auto& info = mCencInfo[i];

Nice use of auto!

@@ +646,5 @@
> +            return ERROR_IO;
> +        }
> +        offset += sizeof(info.mSubsampleCount);
> +
> +        if (size < IV_BYTES + sizeof(info.mSubsampleCount) + info.mSubsampleCount * 6) {

What has following local style done to us??? Aaarrghh

@@ +1086,5 @@
> +        ALOGE("cenc info requested for out of range sample index");
> +        return ERROR_MALFORMED;
> +    }
> +
> +    auto& info = mCencInfo[aSampleIndex];

Suggestion: setCapacity()

Comment 2

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/031f438aaa23

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/031f438aaa23