Add luma & chroma bit depths to Format #491

dsparano · 2023-06-29T10:58:55Z

Issue
Videos may be encoded with different bit depths (even between luma and chroma), not just the traditional 8 bpp.

Currently the Format class carries Color metadata, but not bit depths. The capability to decode wide bit depth videos is down to the codec profile support from the device but not always the video profile is associated with a unique bit depth (for ex. not for H266 where Main 10 is for both 8 and 10 bpp). Besides working out the bitdepth from the codec specific profile strings would be cumbersome.

We think it would be useful for e.g. renderers, transformers etc to know the video bit depth. We had this issue internally while working on use cases such as 8bpp base and 10bpp LCEVC enhanced output, but we think it could very well benefit other scenarios.

Proposed solution
Add luma and chroma bit depths to the Format class, set them from the various parsers (see changes).

dsparano · 2023-07-07T10:31:08Z

Hi Michael, please let me know if you need any additional info from me for this PR, thanks.

dsparano · 2023-07-18T11:01:12Z

Hi @microkatz any progress on this?

microkatz · 2023-07-19T10:10:35Z

@dsparano

Thank you for your patience. I'm still reviewing the CL. I understand that you would like to have this data through H264 and H265 streams. In that regard, would you update the processH264FmtpAttribute and processH265FmtpAttribute methods in RtspMediaTrack.java to include your luma and chroma data in the Format building?

dsparano · 2023-07-19T11:03:53Z

Thanks Michael for reviewing this. I've added setting bit depths for H264 and H265 in RtspMediaTrack.java, I've also rebased against today's tip of main, just in case.

microkatz · 2023-07-21T12:59:49Z

libraries/extractor/src/main/java/androidx/media3/extractor/mp4/AtomParsers.java

@@ -1237,6 +1247,22 @@ private static void parseVideoSampleEntry(
 } else if (childAtomType == Atom.TYPE_av1C) {
 ExtractorUtil.checkContainerInput(mimeType == null, /* message= */ null);
 mimeType = MimeTypes.VIDEO_AV1;
+ parent.setPosition(childStartPosition + Atom.HEADER_SIZE);


Is there a reason that you move the parser before checking that you are going to read into it at all? You might as well check the condition before setting the position.

You're right, the setPosition could go inside the conditional block.

Conditional block removed, anyway (see other comment).

microkatz · 2023-07-21T13:03:31Z

libraries/extractor/src/main/java/androidx/media3/extractor/mp4/AtomParsers.java

+ int byte2 = parent.readUnsignedByte();
+ int seqProfile = byte2 >> 5;
+ int byte3 = parent.readUnsignedByte();
+ boolean highBitdepth = ((byte3 >> 6) & 0b1) != 0;


The twelveBit boolean condition is only needed if (seqProfile == 2 && highBitdepth). If you could move this assignment into that segment.

The AV1 spec is mentioned at line 1258 to describe how to set bitdepthLuma from seqProfile and highBitdepth, however these two are read from AV1CodecConfigurationRecord (from https://aomediacodec.github.io/av1-isobmff/#av1codecconfigurationbox-syntax) which looks like this:

aligned (8) class AV1CodecConfigurationRecord { unsigned int (1) marker = 1; unsigned int (7) version = 1; unsigned int (3) seq_profile; unsigned int (5) seq_level_idx_0; unsigned int (1) seq_tier_0; unsigned int (1) high_bitdepth; unsigned int (1) twelve_bit; unsigned int (1) monochrome; unsigned int (1) chroma_subsampling_x; unsigned int (1) chroma_subsampling_y; unsigned int (2) chroma_sample_position; unsigned int (3) reserved = 0; unsigned int (1) initial_presentation_delay_present; if (initial_presentation_delay_present) { unsigned int (4) initial_presentation_delay_minus_one; } else { unsigned int (4) reserved = 0; } unsigned int (8) configOBUs[]; }

so I think the added lines here are correct. I could still add the link to the AV1CodecConfigurationRecord spec.

Sorry, misunderstood your comment. Yes, twelveBit can be set before is needed.

microkatz · 2023-07-21T13:10:36Z

libraries/extractor/src/main/java/androidx/media3/extractor/mp4/AtomParsers.java

@@ -1237,6 +1247,22 @@ private static void parseVideoSampleEntry(
 } else if (childAtomType == Atom.TYPE_av1C) {
 ExtractorUtil.checkContainerInput(mimeType == null, /* message= */ null);
 mimeType = MimeTypes.VIDEO_AV1;
+ parent.setPosition(childStartPosition + Atom.HEADER_SIZE);
+ if (childAtomSize > Atom.HEADER_SIZE) {


What is the reason behind using this as the condition to see if an AV1CodecConfigurationBox is included?

This comes from a fix we made in our fork, where we found encodes that had missing HEVCDecoderConfigurationRecord so we added that check for all such atoms. However given that the specs, both ISO 14496-15 and the AV1 one, say "sample entry shall contain an ...Configuration Box" it seems such encodes would not be compliant.
Long story short I think you're right, I should remove the check at line 1251.

microkatz · 2023-07-21T13:11:10Z

libraries/common/src/main/java/androidx/media3/common/Format.java

@@ -160,6 +162,8 @@ public static final class Builder {
 @Nullable private byte[] projectionData;
 private @C.StereoMode int stereoMode;
 @Nullable private ColorInfo colorInfo;
+ private int lumaBitdepth;


Could you please instead add this bit depth info to the ColorInfo class?

I don't have a strong opinion on where this value should go, but I figure both the luma and chroma bitDepth should be in the same class (either ColorInfo or Format)

Yes, my apology, the comment was to move both into ColorInfo. Could not attach the comment to multiple lines.

Tbh it feels slightly wrong to me seeing luma bitdepth in ColorInfo, and since luma & chroma bitdepth are logically in pair I would think they should go in Format, however I'm happy to move them to ColorInfo if you guys prefer so.

@microkatz final decision ColorInfo or Format?

microkatz · 2023-07-21T13:21:18Z

libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/RtspMediaTrack.java

Since you are adding the luma and chroma information to vp9 in the mp4 AtomParsers, if you could please at the parsing logic for vp9 for rtsp in this file as well. It will require parsing the fmtp parameters if they exist.

Sure, looking into this...

If I'm correct from https://www.ietf.org/archive/id/draft-ietf-payload-vp9-16.txt the RTP payload for VP9 is not final yet. In the current draft the fmtp parameters for VP9 seem only optional: "max-fr", "max-fs" and "profile-id". None of them allows setting the bit depths. Incidentally I've played with mediamtx and I see it doesn't add fmtp at all for VP9.
Anyway, I can add a processVP9FmtpAttribute() method.

Well, in more detail: I could add it, but it seems to me we couldn't get anything useful out of it, the only possibly relevant bit is the profile-id however for values 0 and 1 the bit depth is 8, which is already the default in Format.Builder, while for values of 2 and 3 the bit depth can be either 10 or 12, so we couldn't set any value. So @microkatz can we skip this?

dway123 · 2023-07-21T13:38:01Z

Hello @dsparano , would the Format.ColorInfo's ColorTransfer and ColorSpace values work to identify the bit depth?

My understanding is that 10-bit color is typically used with HDR, whereas 8-bit is typically used with SDR. Therefore, you could identify HDR-ness or 10-bit usage by checking whether the color transfer is COLOR_TRANSFER_ST2084 || COLOR_TRANSER_HLG, or ColorTransfer.isTransferHdr.

ColorSpace may also be able to identify the bit depth, but I'm less certain on that.

dsparano · 2023-07-21T17:57:11Z

Hi @dway123, I think we cannot deny there is a correlation between colour metadata and bit depth, however an SDR content can still be on 10 bit or even more, and also HDR content can be encoded with 10, 12 or more (even 8, depending on how you define "HDR"). So I don't think the transfer characteristics itself, e.g. PQ or HLG, or color primaries should be used to assign a bit depth, not to mention that both H264 and HEVC allow for chroma to have a different bit depth than luma.

dway123 · 2023-07-24T10:57:23Z

Thanks @dsparano. Yeah, I'm aware that 8-bit HDR content can exist (but admittedly just haven't seen it generated myself yet). Have you seen HDR content with a bit depth other than 10-bit?

I wasn't sure if bit-depth might be a proxy for HDR-ness here, but it seems this isn't the case. I don't think this is duplicating already-signalled info then.

I'm also unsure whether this benefits ExoPlayer/Transformer yet, as I haven't seen a use-case myself (the MediaFormat used to configure MediaCodec doesn't use this), but I'm happy to defer to @microkatz on that.

dsparano · 2023-07-26T16:57:14Z

Hi @microkatz sorry to be nagging, if could kindly reply to the two outstanding questions (all details in the relevant comments) I'll then leave you in peace 😉

ColorInfo or Format?
Skip processVP9FmtpAttribute()?

microkatz · 2023-07-26T17:19:10Z

@dsparano
No apology needed. We really appreciate your contribution to the library. I still need to review your PR but I have not forgotten!

As far as answers to your question.

ColorInfo please. I understand your comment about luma not being colorinfo but I think that chromaBitDepth does fit in that class and they should be together. Perhaps if more data is added to that class then it can be renamed in the future as it is an @unstable annotated class.
I read your comments about the fmtp parameters. You can skip the processVP9FmtpAttribute()

dsparano · 2023-07-27T14:53:58Z

Cool, no problem. I've moved the bit depths to ColorInfo, which opens the question of how to consider it "valid", atm I've tried to keep existing behaviour and at the same time allow for bit depths to be still reported in the debug text in case they are available and the colour metadata are not. Please check if you're happy with it.

microkatz · 2023-08-17T09:47:26Z

Hello @dsparano, I was working on setting this PR up for internal review and merging. Unfortunately it seems that this commit is being made from a topic branch off of a private organization-owned fork? Would it be possible to open a new PR from an individual-owned fork? We can't push changes to organization-owned forks like this one. If that's not possible then we can still merge this PR but it will result in an 'evil' merge. See more info here: https://github.com/androidx/media/blob/release/CONTRIBUTING.md#push-access-to-pr-branches

libraries/common/src/main/java/androidx/media3/common/ColorInfo.java

dsparano · 2023-08-30T11:40:37Z

@microkatz It's been quiet for a while, is anything else needed for this to go in?

microkatz · 2023-09-01T14:21:27Z

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!

dsparano · 2023-09-15T12:27:01Z

@microkatz has the internal review completed?

microkatz · 2023-09-15T12:28:48Z

@dsparano. Not just yet. Much of team is currently at a summit and will return next week. We are still working on it. Thank you for your patience!

dsparano · 2023-10-12T14:54:53Z

@microkatz Hi there, is anything else needed from our side on this PR?

microkatz · 2023-10-12T15:02:38Z

I'm still working on it. Your code change breaks quite a number of dependencies using the ColorInfo class. It has required a couple of other linked changes like c12fb67. I appreciate your patience.

…ntainers and parameter sets

…r usage

PiperOrigin-RevId: 574129451 (cherry picked from commit 009d48a)

microkatz self-assigned this Jul 3, 2023

dsparano force-pushed the dsparano-exo128 branch from 1175b47 to 5178488 Compare July 19, 2023 10:39

microkatz reviewed Jul 21, 2023

View reviewed changes

microkatz reviewed Aug 17, 2023

View reviewed changes

libraries/common/src/main/java/androidx/media3/common/ColorInfo.java Outdated Show resolved Hide resolved

microkatz reviewed Aug 17, 2023

View reviewed changes

libraries/common/src/main/java/androidx/media3/common/ColorInfo.java Outdated Show resolved Hide resolved

microkatz force-pushed the dsparano-exo128 branch from 0908b72 to e42635b Compare September 1, 2023 14:21

microkatz force-pushed the dsparano-exo128 branch from 69a8b07 to cd4c5b4 Compare September 21, 2023 12:34

microkatz force-pushed the dsparano-exo128 branch 5 times, most recently from 1e81dcc to d0b84da Compare October 13, 2023 11:28

Add luma and chroma bit depths to Format, set them as signalled in co…

e71ca5a

…ntainers and parameter sets

dsparano and others added 14 commits October 17, 2023 11:42

Set bitdepths in test Format

c7fe066

Replace literal with constant variable

fc61d96

Set luma and chroma bit depths in Format from Rtsp

d454bd0

Improve isobmff atom parsing changes

59ff0db

Move luma and chroma bitdepths to ColorInfo class

f8d9886

Format with google-java-format

ee1b150

Format with google-java-format

c72ec74

Resume toLogString() in ColorInfo; revert DebugTextViewHelper

1fe9c33

Change isColorValid with isDataSpaceValid

f147328

Format with google-java-format

bbcf37d

Updated to use ColorInfo.Builder() and formatting issues

d206be7

Removed deprecated ColorInfo constructor call and fixed formatting

212e138

Updated golden file tests and removed deprecated ColorInfo constructo…

e28288f

…r usage

Updated golden file tests and readded constructor

7162407

microkatz force-pushed the dsparano-exo128 branch from d0b84da to 7162407 Compare October 17, 2023 11:43

copybara-service bot merged commit 009d48a into androidx:main Oct 17, 2023
1 check passed

rohitjoins pushed a commit that referenced this pull request Oct 23, 2023

Merge pull request #491 from v-novaltd:dsparano-exo128

29846d0

PiperOrigin-RevId: 574129451 (cherry picked from commit 009d48a)

haixia-meta mentioned this pull request Oct 25, 2023

Add AV1 color space parsing in MP4 atom parser #692

Merged

androidx locked and limited conversation to collaborators Dec 17, 2023

Add luma & chroma bit depths to Format #491

Add luma & chroma bit depths to Format #491

Conversation

dsparano commented Jun 29, 2023

dsparano commented Jul 7, 2023

dsparano commented Jul 18, 2023

microkatz commented Jul 19, 2023 • edited

dsparano commented Jul 19, 2023

Choose a reason for hiding this comment

dsparano Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsparano Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsparano Jul 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dway123 commented Jul 21, 2023

dsparano commented Jul 21, 2023

dway123 commented Jul 24, 2023

dsparano commented Jul 26, 2023

microkatz commented Jul 26, 2023 • edited

dsparano commented Jul 27, 2023

microkatz commented Aug 17, 2023

dsparano commented Aug 30, 2023

microkatz commented Sep 1, 2023

dsparano commented Sep 15, 2023

microkatz commented Sep 15, 2023

dsparano commented Oct 12, 2023

microkatz commented Oct 12, 2023 • edited

microkatz commented Jul 19, 2023 •

edited

dsparano Jul 24, 2023 •

edited

dsparano Jul 24, 2023 •

edited

dsparano Jul 25, 2023 •

edited

microkatz commented Jul 26, 2023 •

edited

microkatz commented Oct 12, 2023 •

edited