Skip to content

Conversation

@Zaggy1024
Copy link
Contributor

All properties not related to the pitm box have been moved from mp4parse_avif_get_image to mp4parse_avif_get_info. New fields for the color and alpha tracks are included in Mp4parseAvifInfo. To get the sample indices for those tracks, mp4parse_avif_get_indice_table was added as well.

These changes break C API backwards compatibility due to removing fields from Mp4parseAvifImage, so the version is bumped to 0.16.0.

I've implemented AVIF sequence decoding into Firefox using these changes, so they have been tested a bit at least. However, it's worth noting that I don't have access to the MIAF spec due to the price to buy it, so if anyone is able to confirm that anything related to that spec is sane that would be great.

I've also added the pict and auxv track handlers, which could potentially be fleshed out to verify validity, but I don't think that doing so would provide a tangible benefit to end users since it seems like libavif just uses the logic I've implemented for finding color and alpha tracks here.

I would imagine some new tests for these changes would be in order, but I'm unfortunately not familiar with cargo yet, so if those are desired, any pointers would to documentation would be helpful.

@Zaggy1024 Zaggy1024 force-pushed the avifs branch 2 times, most recently from b066b81 to c220ac9 Compare September 7, 2022 22:40
@padenot padenot requested a review from kinetiknz September 8, 2022 14:16
@ChunMinChang
Copy link
Member

It seems these will be imported to gecko in BMO 1788119

@Zaggy1024
Copy link
Contributor Author

Bumping this since I've updated the tests that are relevant to AVIS support to pass with these changes.

Let me know if there are any new tests I should add as well, I'd be happy to add more coverage.

Copy link
Collaborator

@kinetiknz kinetiknz left a comment

Choose a reason for hiding this comment

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

Apologies for not reviewing this in a timely manner, and thanks for working on this!

@Zaggy1024
Copy link
Contributor Author

Thanks for the review! I've fixed the issues you mentioned, and squashed the "Moved pitm" commit into the first commit since it was mainly a bit of a fixup of that commit.

Should be good to go if you don't see any more issues!

@Zaggy1024
Copy link
Contributor Author

Looks like I missed a couple formatting issues. I fixed those, ran cargo fmt locally and force-pushed again, so hopefully it should be all good now.

All properties not related to the `pitm` box have been moved from mp4parse_avif_get_image to mp4parse_avif_get_info. New fields for the color and alpha tracks are included in Mp4parseAvifInfo. To get the sample indices for those tracks, mp4parse_avif_get_indice_table was added as well.

These changes break C API backwards compatibility due to removing fields from Mp4parseAvifImage, so the version is bumped to 0.16.0.
All tests now pass with the removed unsupported flags for files with AVIS branding.
@Zaggy1024
Copy link
Contributor Author

Sorry for the multiple CI attempts, this is my first time working with cargo so I wasn't aware of clippy. Hopefully I've fixed it so that it passes linting now.

@kinetiknz
Copy link
Collaborator

No problem, thanks for the quick follow-ups. Don't worry about the nightly-only clippy failures, I'll sort them out separately.

@kinetiknz kinetiknz merged commit 21381f9 into mozilla:master Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants