Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Dolby Vision Transcoding and Editing Support #1235

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ybai001
Copy link
Contributor

@ybai001 ybai001 commented Apr 2, 2024

This code contribution adds Dolby Vision transcoding and editing support to transformer.

@rohitjoins rohitjoins requested review from droid-girl and removed request for droid-girl April 3, 2024 12:17
@SheenaChhabra
Copy link
Contributor

@ybai001 I just wanted to update that due to other high priority work, it might take a few weeks to review this change. I appreciate your patience.

@ybai001
Copy link
Contributor Author

ybai001 commented May 9, 2024

@ybai001 I just wanted to update that due to other high priority work, it might take a few weeks to review this change. I appreciate your patience.

Sure, understood.

@@ -135,6 +135,9 @@ public DefaultCodec createForVideoDecoding(
if (SDK_INT >= 31 && requestSdrToneMapping) {
mediaFormat.setInteger(
MediaFormat.KEY_COLOR_TRANSFER_REQUEST, MediaFormat.COLOR_TRANSFER_SDR_VIDEO);
} else if (SDK_INT >= 31 && MimeTypes.VIDEO_DOLBY_VISION.equals(format.sampleMimeType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally write the condition in reversed way format.sampleMimeType.equals(MimeTypes.VIDEO_DOLBY_VISION)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's lots of occurrences of the 'yoda condition' in our codebase, but I agree with @SheenaChhabra it is now discouraged in new code. In this case, because Format.sampleMimeType is @Nullable you will need to use Objects.equals(format.sampleMimeType, MimeTypes.VIDEO_DOLBY_VISION).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -135,6 +135,9 @@ public DefaultCodec createForVideoDecoding(
if (SDK_INT >= 31 && requestSdrToneMapping) {
mediaFormat.setInteger(
MediaFormat.KEY_COLOR_TRANSFER_REQUEST, MediaFormat.COLOR_TRANSFER_SDR_VIDEO);
} else if (SDK_INT >= 31 && MimeTypes.VIDEO_DOLBY_VISION.equals(format.sampleMimeType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share some documentation which explains this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dolby Vision is not only a HDR type but also a media type. But it is based on H.264, H.265 or AV1. That is to say, the Dolby Vision stream is base encoded data + Dolby Vision metadata. Dolby Vision decoder will do two things in playback use case: (1) Call base codec, e.g. H.265 decoder, to decode the base encoded data; (2) Apply Dolby Vision metadata to decoded base data (output of step 1) to get Dolby Vision experience.

In editing use case, Dolby Vision decoder does step 1 only. We don't apply the Dolby Vision metadata because the result of this process is related to the screen characteristic and environment brightness. For editing use case, the Dolby Vision metadata will be generated by Dolby Vision encoder. For transcoding use case, this is a trade off between better quality and consistent result on different devices.

By default, Dolby Vision decoder will do both above step 1 & 2. The purpose of
mediaFormat.setInteger(MediaFormat.KEY_COLOR_TRANSFER_REQUEST, MediaFormat.COLOR_TRANSFER_HLG);
is to notify Dolby Vision decoder to do step 1 only.

There is no open specification to explain above logic. But I can share some documentations about Dolby Vision.

  1. Dolby Vision official site: https://www.dolby.com/technologies/dolby-vision/
  2. Dolby Vision specification and white paper: https://professionalsupport.dolby.com/s/specifications-and-white-papers?language=en_US

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For transcoding use case, this is a trade off between better quality and consistent result on different devices.

Which trade off you are talking about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For transcoding use case, this is a trade off between better quality and consistent result on different devices.

Which trade off you are talking about?

For transcoding use case, ideally, we should do above step 1 & 2 for decoding then encoding to another video format. This is "better quality". In practice (in this contribution), we do step 1 only for decoding. The reason is the result of step 2 is related to the screen characteristic and environment brightness. That means user will get different results on different devices. The transcoding result is not "consistent". So "ignoring step 2" is a trade off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not sure of this change. if the decoder givens us dynamic metadata then I think the pipeline will simple ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try your code after removing this code and see how it works? As per my knowledge Transformer should ignore the additional metadata.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the transformer already supports decoding of DV so I wonder why we need any changes in the decoder factory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems unresolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important for DV editing/transcoding use case. As explained in my first comment,

Dolby Vision is not only a HDR type but also a media type. But it is based on H.264, H.265 or AV1. That is to say, the Dolby Vision stream is base encoded data + Dolby Vision metadata. Dolby Vision decoder will do two things in playback use case: (1) Call base codec, e.g. H.265 decoder, to decode the base encoded data; (2) Apply Dolby Vision metadata to decoded base data (output of step 1) to get Dolby Vision experience.

In editing use case, Dolby Vision decoder does step 1 only. We don't apply the Dolby Vision metadata because the result of this process is related to the screen characteristic and environment brightness. For editing use case, the Dolby Vision metadata will be generated by Dolby Vision encoder. For transcoding use case, this is a trade off between better quality and consistent result on different devices.

By default, Dolby Vision decoder will do both above step 1 & 2. This is correct for playback use case.
But for editing/transcoding use case, Dolby Vision decoder to do step 1 only. (ignoring Dolby Vision metadata.) That is the purpose of
mediaFormat.setInteger(MediaFormat.KEY_COLOR_TRANSFER_REQUEST, MediaFormat.COLOR_TRANSFER_HLG);
which notifies Dolby Vision decoder to do step 1 only.

Does this answer your question?

@@ -108,6 +111,11 @@ public int addTrack(Format format) throws MuxerException {
if (isVideo) {
mediaFormat = MediaFormat.createVideoFormat(sampleMimeType, format.width, format.height);
MediaFormatUtil.maybeSetColorInfo(mediaFormat, format.colorInfo);
if (sampleMimeType.equals(MimeTypes.VIDEO_DOLBY_VISION) &&
android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.TIRAMISU) {
mediaFormat.setInteger(MediaFormat.KEY_PROFILE, getDvProfile(format));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please share some corresponding spec which explains these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as Dolby Vision and Profile, the spec is here: https://professionalsupport.dolby.com/s/article/What-is-Dolby-Vision-Profile?language=en_US

And I'd like to add some explanation why I did this code change:

In Media3 library code FrameworkerMuxer.java, addTrack(Format format) method, it calls system MediaMuxer.addTrack(mediaFormat) method without PROFILE/LEVEL settings.

This is OK for other video formats but results in issue for Dolby Vision. The key point is in Android AOSP utils.cpp file, convertMessageToMetadata() method, KEY_PROFILE is checked. If there is no this parameter, BAD_VALUE will be returned.

see: https://cs.android.com/android/platform/superproject/main/+/main:frameworks/av/media/libstagefright/Utils.cpp;drc=216a97bdad3c0f492094250b33d82abf27098932;l=2115

Copy link
Contributor

@SheenaChhabra SheenaChhabra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have some tests cases around these changes.

@ybai001
Copy link
Contributor Author

ybai001 commented Jun 18, 2024

We need to have some tests cases around these changes.

What can I do for this request? At least, I can provide test stream. If you mean I need to implement some test cases, could you guide me what kinds of test cases I should add. Previously, I added some DD+JOC and AC-4 test cases for my media3 contributions so that I am familiar with that process. But for transformer, this is my first contribution.

@SheenaChhabra
Copy link
Contributor

We need to have some tests cases around these changes.

What can I do for this request? At least, I can provide test stream. If you mean I need to implement some test cases, could you guide me what kinds of test cases I should add. Previously, I added some DD+JOC and AC-4 test cases for my media3 contributions so that I am familiar with that process. But for transformer, this is my first contribution.

We could include following tests:

  1. Transmuxing a dolby vision input file should preserve the dolby vision data. This could be a robolectric test. For this, you can refer tests in MediaItemExportTest.java
  2. Transcoding a normal H.264/H.265 to dolby vision. For this your can refer tests in TransformerEndToEndTest.java
  3. Editing a dolby vision input. For this your can refer tests in TransformerEndToEndTest.java

I would also suggest to run these scenario using Transformer demo app. This is to ensure that it works as expected.

@ybai001
Copy link
Contributor Author

ybai001 commented Jul 2, 2024

We need to have some tests cases around these changes.

What can I do for this request? At least, I can provide test stream. If you mean I need to implement some test cases, could you guide me what kinds of test cases I should add. Previously, I added some DD+JOC and AC-4 test cases for my media3 contributions so that I am familiar with that process. But for transformer, this is my first contribution.

We could include following tests:

  1. Transmuxing a dolby vision input file should preserve the dolby vision data. This could be a robolectric test. For this, you can refer tests in MediaItemExportTest.java
  2. Transcoding a normal H.264/H.265 to dolby vision. For this your can refer tests in TransformerEndToEndTest.java
  3. Editing a dolby vision input. For this your can refer tests in TransformerEndToEndTest.java

I would also suggest to run these scenario using Transformer demo app. This is to ensure that it works as expected.

Hi, @SheenaChhabra,
Thanks for your suggestion. As far as the three tests you mentioned,

  1. I checked the file MediaItemExportTest.java and there are several test cases for transmux. But all of them are general test cases. None of them is for specific video format, e.g. AV1. Could you help to clarify why we need to add test case for Dolby Vision but not for other video format? And which test case I should refer to? Instead, I find there is a case in https://github.com/androidx/media/blob/main/libraries/transformer/src/androidTest/java/androidx/media3/transformer/mh/HdrEditingTest.java#L79. Should I create a case for Dolby Vision here?
  2. I have added the case as you mentioned. I created it based on this one (for AV1) https://github.com/androidx/media/blob/main/libraries/transformer/src/androidTest/java/androidx/media3/transformer/TransformerEndToEndTest.java#L1517
  3. I checked the file TransformerEndToEndTest.java. But none of them is related to HDR editing. Instead, I find there is a Dolby Vision editing case in https://github.com/androidx/media/blob/main/libraries/transformer/src/androidTest/java/androidx/media3/transformer/mh/HdrEditingTest.java#L191. Is it enough for your requirement?

Finally,
(1) I added export_transmuxDolbyVisionFile() in HdrEditingTest.java to match your requirement 1;
(2) I added transcode_withOutputVideoMimeTypeDolbyVision_completesSuccessfully() in TransformerEndToEndTest.java to match your requirement 2;
(3) I applied exist exportAndTranscode_dolbyVisionFile_whenHdrEditingIsSupported() and created exportAndTranscode_dolbyVisionFile_whenHdrEditingUnsupported_toneMapsOrThrows() in HdrEditingTest.java to match your requirement 3

If this solution is OK, I'll upload my code.

@ybai001
Copy link
Contributor Author

ybai001 commented Jul 3, 2024

I have uploaded the test cases code.

@@ -1280,6 +1280,39 @@ public void transcode_withOutputVideoMimeTypeAv1_completesSuccessfully() throws
assertThat(exportResult.videoMimeType).isEqualTo(MimeTypes.VIDEO_AV1);
}

@Test
public void transcode_withOutputVideoMimeTypeDolbyVision_completesSuccessfully() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On which device you have tested this which can produce dolby vision metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OPPO flagship series (e.g OPPO Find X6/X7 pro/ultra), ViVo flagship series (e.g. ViVo X90/pro/ultra, X100/pro/ultra) and XiaoMi flagship series.
I tested it on my Pixel phone with Dolby Vision codec integration.

.run(testId, editedMediaItem);
ExportResult exportResult = exportTestResult.exportResult;

assert exportTestResult.filePath != null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this assert sentence because Android Studio had a warning message. I can remove it if you are fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove it based on your latest comment.

level = MediaCodecInfo.CodecProfileLevel.DolbyVisionLevelFhd24; // Level 03
} else if (maxWidthHeight <= 2560 && pps <= 62208000) {
level = MediaCodecInfo.CodecProfileLevel.DolbyVisionLevelFhd30; // Level 04
} else if (maxWidthHeight <= 3840) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for different purpose. The method in your link is to PARSE the Dolby Vision profile and level from codec string. The use case is to play a streaming content. You can get codec string from a MPEG-DASH manifest file. Then paring this codec string, you will get profile/level info. This info can be used to select which Dolby Vision decoder to handle this streaming content (H.264 based Dolby Vision decoder or H.265 based Dolby Vision decoder). In this case, Format.codecs is a valid codec string.

As far as this code contribution case, it is used to decide which Dolby Vision profile/level should be used based on incoming content width/height/framerate. In this case. Format.codecs is NULL.

@@ -108,6 +111,11 @@ public int addTrack(Format format) throws MuxerException {
if (isVideo) {
mediaFormat = MediaFormat.createVideoFormat(sampleMimeType, format.width, format.height);
MediaFormatUtil.maybeSetColorInfo(mediaFormat, format.colorInfo);
if (sampleMimeType.equals(MimeTypes.VIDEO_DOLBY_VISION) &&
android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.TIRAMISU) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change it to pattern similar to SDK_INT >= 33 to make it consistent with other code in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

case MimeTypes.VIDEO_DOLBY_VISION:
if (colorTransfer == C.COLOR_TRANSFER_HLG) {
return ImmutableList.of(
MediaCodecInfo.CodecProfileLevel.DolbyVisionProfileDvheSt);
Copy link
Contributor

@SheenaChhabra SheenaChhabra Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of all profile, why have we chosen only this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. All exist devices which support Dolby Vision encoding on market will encode the input stream to this profile only. In future, maybe we will extend this list. But Dolby has not finalized which profiles we should support for encoding.

@@ -139,6 +139,41 @@ public void export_transmuxHlg10File() throws Exception {
assertThat(actualColorTransfer).isEqualTo(C.COLOR_TRANSFER_HLG);
}

@Test
public void export_transmuxDolbyVisionFile() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think transmuxing test case can also go into MediaItemExportTest as it does not need to run on an actual device.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give me more hint about it?

  1. I added the Dolby Vision test case here because there are export_transmuxHdr10File() and export_transmuxHlg10File() test cases in this file. I did the similar thing in my test case implementation.
  2. In MediaItemExportTest, I found some transmux test cases but I didn't find corresponding test cases for HLG and HDR10. Do I need to add test case for Dolby Vision in this file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests needs to run on real device because in Robolectric tests, FrameworkMuxer does not write to the actual output file and we are doing validations on the actual output file. So we should keep the test case here only. Thanks!


if (Util.SDK_INT < 24) {
// TODO: b/285543404 - Remove suppression once we can transmux H.265/HEVC before API 24.
recordTestSkipped(context, testId, /* reason= */ "Can't transmux H.265/HEVC before API 24");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The below check for format support should check whether muxer supports this format or not so this is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did the same thing as export_transmuxHdr10File() and export_transmuxHlg10File(). Shall we remove above code in these three methods together?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why existing tests are written in that way but as per my understanding for transmuxing decoder/encoder should not get involved. So the below check for decoder should not be there. We should definitely check for muxer, which is done separately here and should remain.

You can leave the existing test as it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not fully resolved as the decoder check (AndroidTestUtil.skipAndLogIfFormatsUnsupported) is still present which should not be required for transmuxing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll remove this part of checking.

if (AndroidTestUtil.skipAndLogIfFormatsUnsupported(
    context,
    testId,
    /* inputFormat= */ MP4_ASSET_DOLBY_VISION_HDR_FORMAT,
    /* outputFormat= */ null)) {
  return;
}

new TransformerAndroidTestRunner.Builder(context, transformer)
.build()
.run(testId, mediaItem);
assert exportTestResult.filePath != null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally use assertThat(xyz).isNotNull();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your latest comment, I'll remove this sentence.

assert exportTestResult.filePath != null;
@C.ColorTransfer
int actualColorTransfer =
Objects.requireNonNull(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

retrieveTrackFormat(context, exportTestResult.filePath, C.TRACK_TYPE_VIDEO)
.colorInfo)
.colorTransfer;
assertThat(actualColorTransfer).isEqualTo(C.COLOR_TRANSFER_HLG);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check the mime type as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

@C.ColorTransfer
int actualColorTransfer =
retrieveTrackFormat(context, exportTestResult.filePath, C.TRACK_TYPE_VIDEO)
.colorInfo
Objects.requireNonNull(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because there is a warning message from latest Android Studio. I can revert this change if you are fine.

Copy link
Contributor

@SheenaChhabra SheenaChhabra Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR; you can remove these and other null checks from the test code.

Just had a discussion with my team about these warnings and we have decided to ignore these. This is because we have other internal tools to check these nullness issue (although we have disabled them for test to avoid null check overhead).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this checking as you latest comment.

assert format.colorInfo != null;
if (deviceSupportsHdrEditing(VIDEO_H265, format.colorInfo)) {
recordTestSkipped(context, testId, /* reason= */ "Device supports Dolby Vision editing.");
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can possibly use assumeDeviceDoesNotSupportHdrEditing() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not familiar with resolving confliction on GitHub. Let's complete other issues then handle the confliction issue. (I fork the main branch at the beginning of this code contribution. At that time, there is no this method.)

}

if (AndroidTestUtil.skipAndLogIfFormatsUnsupported(
context, testId, /* inputFormat= */ format, /* outputFormat= */ null)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has been renamed to assumeFormatsSupported(). Sorry for the conflicts.

.addListener(
new Transformer.Listener() {
@Override
public void onFallbackApplied(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is deprecated, use the alternative.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll change the code to use

default void onFallbackApplied(
    Composition composition,
    TransformationRequest originalTransformationRequest,
    TransformationRequest fallbackTransformationRequest) {
  MediaItem mediaItem = composition.sequences.get(0).editedMediaItems.get(0).mediaItem;
  onFallbackApplied(mediaItem, originalTransformationRequest, fallbackTransformationRequest);
}

String actualMimeType =
retrieveTrackFormat(context, exportTestResult.filePath, C.TRACK_TYPE_VIDEO).sampleMimeType;
assertThat(actualMimeType).isEqualTo(MimeTypes.VIDEO_DOLBY_VISION);
assertThat(exportResult.exportException).isNull();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is any exception then test would actually throw so this check is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just follow Line 1278 and 1331 in transcode_withOutputVideoMimeTypeAv1_completesSuccessfully() and transcode_withOutputAudioMimeTypeAac_completesSuccessfully()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why existing tests has this assertion but we can remove it for the newly added tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll remove it.

assertThat(actualMimeType).isEqualTo(MimeTypes.VIDEO_DOLBY_VISION);
assertThat(exportResult.exportException).isNull();
assertThat(exportResult.durationMs).isGreaterThan(0);
assertThat(exportResult.videoMimeType).isEqualTo(MimeTypes.VIDEO_DOLBY_VISION);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are checking the actual mime type, this check can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just followed Line 1280 and 1333 in transcode_withOutputVideoMimeTypeAv1_completesSuccessfully() and transcode_withOutputAudioMimeTypeAac_completesSuccessfully() respectively.

String actualMimeType =
retrieveTrackFormat(context, exportTestResult.filePath, C.TRACK_TYPE_VIDEO).sampleMimeType;
assertThat(actualMimeType).isEqualTo(MimeTypes.VIDEO_DOLBY_VISION);
assertThat(exportResult.exportException).isNull();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is any exception then test would actually throw so this check is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just followed Line 1278 and 1331 in transcode_withOutputVideoMimeTypeAv1_completesSuccessfully() and transcode_withOutputAudioMimeTypeAac_completesSuccessfully() respectively.


if (Util.SDK_INT < 24) {
// TODO: b/285543404 - Remove suppression once we can transmux H.265/HEVC before API 24.
recordTestSkipped(context, testId, /* reason= */ "Can't transmux H.265/HEVC before API 24");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why existing tests are written in that way but as per my understanding for transmuxing decoder/encoder should not get involved. So the below check for decoder should not be there. We should definitely check for muxer, which is done separately here and should remain.

You can leave the existing test as it is.

public void export_transmuxDolbyVisionFile() throws Exception {
Context context = ApplicationProvider.getApplicationContext();

if (Util.SDK_INT < 24) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have now started using assumeTrue(Util.SDK_INT >= 24). By using this the test is marked as skipped instead of marked passed.
So the whole if block check can be replaced with assumeTrue(Util.SDK_INT >= 24)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll change the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use @SdkSuppress(minSdkVersion = 24) if your only condition is SDK level.

https://developer.android.com/reference/androidx/test/filters/SdkSuppress

This is preferable to assumeTrue imo because it skips the test "earlier", and just doesn't run it at all, instead of needing to mark it as 'skipped' or 'passed' (which has mixed support in different test running environments).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think this method is better. I'll change the code again.

@@ -135,6 +135,9 @@ public DefaultCodec createForVideoDecoding(
if (SDK_INT >= 31 && requestSdrToneMapping) {
mediaFormat.setInteger(
MediaFormat.KEY_COLOR_TRANSFER_REQUEST, MediaFormat.COLOR_TRANSFER_SDR_VIDEO);
} else if (SDK_INT >= 31 && MimeTypes.VIDEO_DOLBY_VISION.equals(format.sampleMimeType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try your code after removing this code and see how it works? As per my knowledge Transformer should ignore the additional metadata.

@RequiresApi(33)
private static int getDvProfile(Format format) {
// Currently, only profile 8 is supported for encoding
// TODO: set profile ID based on format.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the work involved for this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, only profile 8 (H.265 based) is supported for encoding. In future, maybe Dolby will support other Dolby Vision profile (e.g. profile 9/10). It's not finalized. If that is true, we need to add logic here to decide which profile should be used based on the parameter "format".

@@ -135,6 +135,9 @@ public DefaultCodec createForVideoDecoding(
if (SDK_INT >= 31 && requestSdrToneMapping) {
mediaFormat.setInteger(
MediaFormat.KEY_COLOR_TRANSFER_REQUEST, MediaFormat.COLOR_TRANSFER_SDR_VIDEO);
} else if (SDK_INT >= 31 && MimeTypes.VIDEO_DOLBY_VISION.equals(format.sampleMimeType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the transformer already supports decoding of DV so I wonder why we need any changes in the decoder factory.

} else if (maxWidthHeight <= 1920 && pps <= 49766400) {
level = MediaCodecInfo.CodecProfileLevel.DolbyVisionLevelFhd24; // Level 03
} else if (maxWidthHeight <= 2560 && pps <= 62208000) {
level = MediaCodecInfo.CodecProfileLevel.DolbyVisionLevelFhd30; // Level 04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why the value of these constants is not same as mentioned level? For example DolbyVisionLevelFhd30 = 8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the codec profile/level constants should be a bitmask, which is defined by Android AOSP. Dolby defined our profile/level to follow this rule. See https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/media/java/android/media/MediaCodecInfo.java;drc=6a4bef0f90e822e19866e53a98b85029bff04ea0;l=4301

I think Google defined it in this way due to efficiency. For example, to judge whether one profile is supported by current device, you just need to do one time comparison:
if (appointedLevel & (DolbyVisionLevelFhd24 | DolbyVisionLevelFhd24) {
// Current device support this level
} else {
// Current device doesn't support this level
}

}

// Get Dolby Vision level
// Refer to https://professionalsupport.dolby.com/s/article/What-is-Dolby-Vision-Profile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write it as a java doc comment. Same comment for other method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@SheenaChhabra
Copy link
Contributor

@ychaparov Can you please help with another review of this PR?

@ychaparov
Copy link
Contributor

Looks good to me. @SheenaChhabra please approve when you think your comments have been resolved

@ybai001
Copy link
Contributor Author

ybai001 commented Aug 6, 2024

@SheenaChhabra, I have updated code as your comments except resolving conflicts. Let me know whether there is any other comment from your side.

Copy link
Contributor

@SheenaChhabra SheenaChhabra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall changes are good but some of the comments are still not resolved.
Once the comments are resolved/answered and the conflicts are resolved, we can take this forward.

String actualMimeType =
retrieveTrackFormat(context, exportTestResult.filePath, C.TRACK_TYPE_VIDEO).sampleMimeType;
assertThat(actualMimeType).isEqualTo(MimeTypes.VIDEO_DOLBY_VISION);
assertThat(exportResult.exportException).isNull();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why existing tests has this assertion but we can remove it for the newly added tests.


if (Util.SDK_INT < 24) {
// TODO: b/285543404 - Remove suppression once we can transmux H.265/HEVC before API 24.
recordTestSkipped(context, testId, /* reason= */ "Can't transmux H.265/HEVC before API 24");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not fully resolved as the decoder check (AndroidTestUtil.skipAndLogIfFormatsUnsupported) is still present which should not be required for transmuxing.

.addListener(
new Transformer.Listener() {
@Override
public void onFallbackApplied(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not resolved.

@@ -135,6 +135,9 @@ public DefaultCodec createForVideoDecoding(
if (SDK_INT >= 31 && requestSdrToneMapping) {
mediaFormat.setInteger(
MediaFormat.KEY_COLOR_TRANSFER_REQUEST, MediaFormat.COLOR_TRANSFER_SDR_VIDEO);
} else if (SDK_INT >= 31 && MimeTypes.VIDEO_DOLBY_VISION.equals(format.sampleMimeType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems unresolved.

@ybai001
Copy link
Contributor Author

ybai001 commented Aug 8, 2024

Overall changes are good but some of the comments are still not resolved. Once the comments are resolved/answered and the conflicts are resolved, we can take this forward.

I have updated the code as you requested and resolved the conflicts. Let's me know whether there is any other issue.

@ybai001
Copy link
Contributor Author

ybai001 commented Aug 8, 2024

After resolving the merge conflict, I find the code base is updated to the latest one and I have to update my test code to match the latest one. (Some definitions are changed totally, e.g. MP4_ASSET.
I'll refine my code and upload it.

@ybai001
Copy link
Contributor Author

ybai001 commented Aug 8, 2024

@SheenaChhabra, I updated code based on latest code on main branch. I think I resolved all the conflicts. Let me know whether you have more questions. Sorry for tons of commits and comments today.

Copy link
Contributor

@SheenaChhabra SheenaChhabra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this change.
I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@ybai001
Copy link
Contributor Author

ybai001 commented Aug 12, 2024

Thanks for making this change. I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

Thanks. BTW, just want to let you know:
Google can't push changes to this organization-owned forks because Dolby didn't grant you collaborator access. I have applied for it to our Dolby admin but she didn't agree with it. As result, you can merge this PR but it will result in an 'evil' merge. See details here: https://github.com/androidx/media/blob/release/CONTRIBUTING.md#push-access-to-pr-branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants