Add AC-4 DRM Support

[ybai001]

• AC-4 DRM MPEG-DASH On-Demand Profile
• AC-4 DRM MPEG-DASH Live Profile
• AC-4 DRM HLSv7 (fmp4) On-Demand Profile
• AC-4 DRM HLSv7 (fmp4) Live Profile

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[ybai001]

@andrewlewis This license issue is same as pull request #5438. (Due to two contributors on same branch.) Could you help to manually verified that the CLAs? Thanks.

[ybai001]

1. Sorry to submit pull request just before your holiday. :(

2. Some technical background of this change:
   Fragmented mp4 Extractor implements a special behavior for AC-4 audio track in MPEG-4 container. (Add AC-4 format support #5303) The extracted AC-4 RAW frame should be prefixed with AC-4 header by extractor to create an AC-4 SYNC frame as output, which is requirement from Dolby.
   The added AC-4 header size is always 7 bytes. Please check "ETSI TS 103 190-1 V1.3.1, Annex G (normative): AC-4 Sync Frame" for details.
   For AC-4 encryption content, since the added AC-4 header is clear data, it results in the outputted sample is neither full clear data nor full encryption data. ExoPlayer can notify this info (clear data size and encrypted data size) to Android framework via MediaCodec.CryptoInfo API. This mechanism can work for other audio formats. But ExoPlayer code needs to be modified for AC-4 case since fragmented mp4 extractor modifies the extracted sample data for AC-4 only.

3. This pull request add AC-4 DRM support for MPEG-DASH and HLSv7(fmp4). Dolby doesn't and won't create HLSv3(ts) for AC-4. As far as HLSv4(packed audio), due to ExoPlayer doesn't support SAMPLE-AES (Support SAMPLE-AES encryption with identity key format #6488), we didn't test this case. Is Google considering add SAMPLE-AES support in ExoPlayer? If not, we may consider doing it.

4. Merry Christmas!

[tonihei]

CLA verified manually. (Committers are ybai and aujohn on corporate CLA.)

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

[tonihei]

@AquilesCanta Could you take a look at this change?

[ojw28]

I can take an initial look at this, to get the ball rolling.

[ojw28]

I put a fairly substantial comment in-line, so please take a look at that. If you agree, I'd suggest first sending a pull request that just moves writing the AC4 header to its new location, both in FragmentedMp4Extractor and Mp4Extractor. This change should be a no-op in terms of functionality. It would also be great if this change could add small test assets for AC4 in MP4/FMP4, which could be tested from Mp4ExtractorTest and FragmentedMp4ExtractorTest. Adding such assets really adds a lot more confidence that no-op changes really are no-op changes! This pull request could then build on top of that one.

[ojw28]

I'm not sure if I'm missing something, but is there a reason not to output the AC4 header directly here, removing the need for isAc4HeaderRequired and outputSampleEncryptionDataSize? The same is true for Mp4Extractor as well, so if you end up doing this, please do it there as well.

I also think you should avoid changing SampleQueue and SampleDataQueue as part of this change. In the current proposal, the sample data is modified in the extractor (by adding the header) but the associated encryption data is not. This means there's a mismatch between the sample data and associated encryption data in the sample queue, which seems wrong to me. The encryption data is then fixed on the reading side of the sample queue. Instead of doing it this way, I think the encryption data should really be fixed on the writing side of the sample queue in the extractor, since that's where the sample data is modified.

If both of these points make sense, I think you can do something like this (replacing the code here, starting from where you need to call outputSampleEncryptionData):

boolean isAc4HeaderRequired =
    MimeTypes.AUDIO_AC4.equals(currentTrackBundle.track.format.sampleMimeType);

int encryptionDataBytesWritten = currentTrackBundle.outputSampleEncryptionData(
    sampleSize, isAc4HeaderRequired ? Ac4Util.SAMPLE_HEADER_SIZE : 0);
sampleBytesWritten = encryptionDataBytesWritten;
sampleSize += encryptionDataBytesWritten;

if (isAc4HeaderRequired) {
  Ac4Util.getAc4SampleHeader(sampleSize, scratch);
  currentTrackBundle.output.sampleData(scratch, Ac4Util.SAMPLE_HEADER_SIZE);
  sampleBytesWritten += Ac4Util.SAMPLE_HEADER_SIZE;
  sampleSize += Ac4Util.SAMPLE_HEADER_SIZE;
}

parserState = STATE_READING_SAMPLE_CONTINUE;
sampleCurrentNalBytesRemaining = 0;

Where TrackBundle.outputSampleEncryptionData corrects the encryption data as it writes it into the sample queue. That will be a bit tricky to get right, but roughly speaking, the bottom half of that method needs to look more like this (with the // TODO bits done):

/**
 * Outputs the encryption data for the current sample.
 *
 * @param sampleSize The size of the current sample in bytes, excluding any additional clear
 *     header that will be prefixed to the sample by the extractor.
 * @param clearHeaderSize The size of a clear header that will be prefixed to the sample by the
 *     extractor, or 0.
 * @return The number of written bytes.
 */
public int outputSampleEncryptionData(int sampleSize, int clearHeaderSize) {

  // .... top half of the method as it is currently. Then:

  boolean haveSubsampleEncryptionTable = 
      fragment.sampleHasSubsampleEncryptionTable(currentSampleIndex);
  boolean writeSubsampleEncryptionData =
      haveSubsampleEncryptionTable | clearHeaderSize != 0;

  // Write the signal byte, containing the vector size and the subsample encryption flag.
  encryptionSignalByte.data[0] =
      (byte) (vectorSize | (writeSubsampleEncryptionData ? 0x80 : 0));
  encryptionSignalByte.setPosition(0);
  output.sampleData(encryptionSignalByte, 1);
  // Write the vector.
  output.sampleData(initializationVectorData, vectorSize);

  if (!writeSubsampleEncryptionData) {
    return 1 + vectorSize;
  }

  if (!haveSubsampleEncryptionTable) {
    // TODO: Need to synthesize subsample encryption data. The sample is fully encrypted except
    // for the additional header that the extractor is going to prefix, so we need to write the
    // following to output.sampleData:
    // subsampleCount (unsigned short) = 1
    // clearDataSizes[0] (unsigned short) = clearHeaderSize
    // encryptedDataSizes[0] (unsigned int) = sampleSize
    return 1 + vectorSize + 8;
  }

  ParsableByteArray subsampleEncryptionData = fragment.sampleEncryptionData;
  int subsampleCount = subsampleEncryptionData.readUnsignedShort();
  subsampleEncryptionData.skipBytes(-2);
  int subsampleDataLength = 2 + 6 * subsampleCount;
  // TODO: On the way through, we need to re-write the 3rd and 4th bytes, which hold
  // clearDataSizes[0], so that clearHeaderSize is added into the value. This must be done
  // without modifying subsampleEncryptionData itself.
  output.sampleData(subsampleEncryptionData, subsampleDataLength);
  return 1 + vectorSize + subsampleDataLength;
}

[ojw28]

Note: I edited this to correct some mistakes, so please look on GitHub rather than relying on the email :).

[ojw28]

See comment elsewhere about not modifying this class at all, however as an aside I don't think you should be assuming there's no sub-sample encryption (i.e. you'd also need to do something in the if block above, as well as the else block here).

[ybai001]

@ojw28 The fact is I added code in if block but I removed it when I submitted this pull request. The reason is there is no this kind of stream inside Dolby so that I can't test that if branch. I can double check with our content creation team whether they will create this kind of stream in future. BTW, according to your experience, is there sub-sample encryption content for audio track (e.g. aac) in practice? Thanks.

[ojw28]

I'm not sure whether sub-sample encryption is really a thing for audio samples, but I think we should handle the possibility :).

[ojw28]

Is Google considering add SAMPLE-AES support in ExoPlayer? If not, we may consider doing it.

We're still treating this as low priority, as per #6488. It's also only becoming less important over time, due to the shift to CMAF. I don't really see much value in adding support at this point.

[ybai001]

Is Google considering add SAMPLE-AES support in ExoPlayer? If not, we may consider doing it.

We're still treating this as low priority, as per #6488. It's also only becoming less important over time, due to the shift to CMAF. I don't really see much value in adding support at this point.

OK, I got your point.

[ybai001]

I put a fairly substantial comment in-line, so please take a look at that. If you agree, I'd suggest first sending a pull request that just moves writing the AC4 header to its new location, both in FragmentedMp4Extractor and Mp4Extractor. This change should be a no-op in terms of functionality. It would also be great if this change could add small test assets for AC4 in MP4/FMP4, which could be tested from Mp4ExtractorTest and FragmentedMp4ExtractorTest. Adding such assets really adds a lot more confidence that no-op changes really are no-op changes! This pull request could then build on top of that one.

OK. I'll submit a new pull request to move writing the AC4 header to its new location and add test assets at the same time. Then I'll update AC-4 DRM pull request.

[ybai001]

I put a fairly substantial comment in-line, so please take a look at that. If you agree, I'd suggest first sending a pull request that just moves writing the AC4 header to its new location, both in FragmentedMp4Extractor and Mp4Extractor. This change should be a no-op in terms of functionality. It would also be great if this change could add small test assets for AC4 in MP4/FMP4, which could be tested from Mp4ExtractorTest and FragmentedMp4ExtractorTest. Adding such assets really adds a lot more confidence that no-op changes really are no-op changes! This pull request could then build on top of that one.

Hi, @ojw28,
I created new pull request (#6836) to move writing the AC4 header to its new location as you suggested. At the same time, I added two test cases for Mp4Extractor and FragmentedMp4Extractor respectively. But there is CLA issue and I don't know why it happens. Could you help to approve it manually? Thanks.

BR,
Yanning

[ojw28]

Regarding the CLA issue:

1. On my side, I can see both Arun and yourself are listed as having signed the CLA under your work email addresses, however your GitHub usernames are missing.
2. The commits in your pull requests don't all use your work email addresses. For example there are commits being made using [email protected] and [email protected].

Since we don't have your GitHub usernames registered, we don't recognize the commits as being from you unless you use your work email addresses. Hence the problem :). There are a few things you can try to fix this:

1. I'm not sure what the UI looks like on your side, but whoever manages your corporate CLA might just be able to log in and add your GitHub usernames, to resolve the problem.
2. You could alternatively, or in addition, make sure your commits always use your work email addresses. There are instructions here, which are different depending on whether you're making changes via the GitHub web UI or Git.

I suspect the first approach would be more robust because it doesn't rely on your configuring Git properly on all your devices.

[ojw28]

The first of your pull requests has been merged (with some modifications to fix the issues highlighted by the tests). Let me know once this one is updated. Thanks for your help!

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[ybai001]

The first of your pull requests has been merged (with some modifications to fix the issues highlighted by the tests). Let me know once this one is updated. Thanks for your help!

Done. Updated code based on your comments. Please help to review it.

Your method is better than my original one. Thank you very much for your detailed instruction and example code.

[ybai001]

Regarding the CLA issue:

1. On my side, I can see both Arun and yourself are listed as having signed the CLA under your work email addresses, however your GitHub usernames are missing.
2. The commits in your pull requests don't all use your work email addresses. For example there are commits being made using [email protected] and [email protected].

Since we don't have your GitHub usernames registered, we don't recognize the commits as being from you unless you use your work email addresses. Hence the problem :). There are a few things you can try to fix this:

1. I'm not sure what the UI looks like on your side, but whoever manages your corporate CLA might just be able to log in and add your GitHub usernames, to resolve the problem.
2. You could alternatively, or in addition, make sure your commits always use your work email addresses. There are instructions here, which are different depending on whether you're making changes via the GitHub web UI or Git.

I suspect the first approach would be more robust because it doesn't rely on your configuring Git properly on all your devices.

Thanks for your warmhearted. I'll try to solve this issue. (Sorry, seem that you need to approve it again since I updated this pull request.)

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

[ojw28]

Is it possible for you to add another test case (FragmentedMp4ExtractorTest.testSampleWithProtectedAc4Track) to make sure we have a test case covering adjustment of the sample crypto data to take the prefixed AC4 header into account?

It's fine to cover just the full-sample-encryption case, given you're not generating audio streams that use sub-sample encryption at the moment (and, therefore, I assume it's non-trivial for you to add such a corresponding test case :)).

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[ybai001]

Is it possible for you to add another test case (FragmentedMp4ExtractorTest.testSampleWithProtectedAc4Track) to make sure we have a test case covering adjustment of the sample crypto data to take the prefixed AC4 header into account?

It's fine to cover just the full-sample-encryption case, given you're not generating audio streams that use sub-sample encryption at the moment (and, therefore, I assume it's non-trivial for you to add such a corresponding test case :)).

Done. And I need to solve CLA issue ASAP. Otherwise, you need to manually approve it again and again. Sorry for that.

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.