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 AV1 color space parsing in MP4 atom parser #692

Merged
merged 9 commits into from Nov 2, 2023

Conversation

haixia-meta
Copy link
Contributor

This adds av1C parsing based on the AV1 bitstream specification: https://aomediacodec.github.io/av1-spec/av1-spec.pdf

This is needed to have correct color space when using hardware AV1 decoder.

@microkatz
Copy link
Contributor

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!

@microkatz
Copy link
Contributor

@haixia-meta

Actually would you be able to add a unit test to the Mp4ExtractorTest file with a small av1 sample with color info for testing purposes?

@haixia-meta
Copy link
Contributor Author

@haixia-meta

Actually would you be able to add a unit test to the Mp4ExtractorTest file with a small av1 sample with color info for testing purposes?

I'd love to but do you have any example or documentation on how to generate the prerecorded dump files associated with the av1 sample mp4? Thanks

@microkatz
Copy link
Contributor

@haixia-meta
Actually would you be able to add a unit test to the Mp4ExtractorTest file with a small av1 sample with color info for testing purposes?

I'd love to but do you have any example or documentation on how to generate the prerecorded dump files associated with the av1 sample mp4? Thanks

If you were to add your test and run the Mp4ExtractorTest, it should fail with an error in DumpFileAsserts. This is due to the default DUMP_FILE_ACTION being COMPARE_WITH_EXISTING. The failure would tell you to change the action to WRITE_TO_LOCAL to generate the dump file. Make sure you change the DUMP_FILE_ACTION back to COMPARE_WITH_EXISTING afterwards though!

@haixia-meta
Copy link
Contributor Author

@haixia-meta
Actually would you be able to add a unit test to the Mp4ExtractorTest file with a small av1 sample with color info for testing purposes?

I'd love to but do you have any example or documentation on how to generate the prerecorded dump files associated with the av1 sample mp4? Thanks

If you were to add your test and run the Mp4ExtractorTest, it should fail with an error in DumpFileAsserts. This is due to the default DUMP_FILE_ACTION being COMPARE_WITH_EXISTING. The failure would tell you to change the action to WRITE_TO_LOCAL to generate the dump file. Make sure you change the DUMP_FILE_ACTION back to COMPARE_WITH_EXISTING afterwards though!

thank you for explaining! forgive my n00b question but how do I run the Mp4ExtractorTest? thanks again

@microkatz
Copy link
Contributor

If you were to add your test and run the Mp4ExtractorTest, it should fail with an error in DumpFileAsserts. This is due to the default DUMP_FILE_ACTION being COMPARE_WITH_EXISTING. The failure would tell you to change the action to WRITE_TO_LOCAL to generate the dump file. Make sure you change the DUMP_FILE_ACTION back to COMPARE_WITH_EXISTING afterwards though!

thank you for explaining! forgive my n00b question but how do I run the Mp4ExtractorTest? thanks again

A simple way to do it is open the project in Android Studio, go to the Mp4ExtractorTest file and click the play symbol next to the class declaration to run the test file. Or you can right-click on the file in the list in the Project-Pane and select "Run 'Mp4ExtractorTest'".

@haixia-meta
Copy link
Contributor Author

@microkatz - as requested, added new test case for Mp4ExtractorTest with sample AV1 test file with color info and the corresponding dump files.

@microkatz
Copy link
Contributor

Hey @haixia-meta,

I ran your unit test and I don't think it is testing the code that you added. When I ran the unit test it appears that the code exits in line 2246 with obu_type != OBU_SEQUENCE_HEADER with obu_type being 0. The colorInfo in your generated unit test files comes from a colr type Atom. Did you notice this as well? Do you have any other samples you can provide?

@haixia-meta
Copy link
Contributor Author

hey @microkatz - apologies; this was broken by a merge with the recently merged #491 which modified the parser byte position

I've updated the parser to skip ahead 1 byte instead of 4 which fixed the issue

Also updated the test mp4 file which manually removed the colr atom to make sure we're parsing color from av1c.

Hey @haixia-meta,

I ran your unit test and I don't think it is testing the code that you added. When I ran the unit test it appears that the code exits in line 2246 with obu_type != OBU_SEQUENCE_HEADER with obu_type being 0. The colorInfo in your generated unit test files comes from a colr type Atom. Did you notice this as well? Do you have any other samples you can provide?

@microkatz
Copy link
Contributor

microkatz commented Oct 25, 2023

Makes sense and great! Glad we caught that.

I noticed your new sample file has more than ten times the number of samples that your old one had. Any chance you can shorten the mp4 sample you just added?

@haixia-meta
Copy link
Contributor Author

Makes sense and great! Glad we caught that.

I noticed your new sample file has more than ten times the number of samples that your old one had. Any chance you can shorten the mp4 sample you just added?

done - I reverted to the previous shorter mp4 file with the "colr" atom manually renamed to "free"

@microkatz
Copy link
Contributor

microkatz commented Oct 31, 2023

@haixia-meta

Thank you for your patience. The code is still in review. If I may ask, where in the av1c spec does the requirement that the obu_size must not be greater than 127? Its in your Parser logic just after reading for obuHasSizeField. Just trying to match the very-specific parsing logic to the AV1 Bitstream & Decoding Process Specification.

@haixia-meta
Copy link
Contributor Author

@haixia-meta

Thank you for your patience. The code is still in review. If I may ask, where in the av1c spec does the requirement that the obu_size must not be greater than 127? Its in your Parser logic just after reading for obuHasSizeField. Just trying to match the very-specific parsing logic to the AV1 Bitstream & Decoding Process Specification.

@microkatz thanks for asking! The obu_size in the spec is a leb128() type. See 4.10.5.: unsigned number represented by a variable number of little-endian bytes - the byte sequence is terminated when the MSB is 0 (i.e. byte value ≤ 127). If we can assume that sequence header will never exceed 127 bytes long, then the parsing of leb128() can be simplified as a f(8).

haixia-meta and others added 4 commits November 2, 2023 15:31
This adds av1C parsing based on the AV1 bitstream specification: https://aomediacodec.github.io/av1-spec/av1-spec.pdf

This is needed to have correct color space when using hardware AV1 decoder.
* Rename method f(n) to a more helpful name.
* Move the private inner class to the bottom.
* Move public fields before private fields but below the static field.
* Make `private ParsableByteArray data` final.
* Make sure `parseSequenceHeader` can only be called once.
@copybara-service copybara-service bot merged commit dab9eb3 into androidx:main Nov 2, 2023
1 check passed
@androidx androidx locked and limited conversation to collaborators Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants