-
Notifications
You must be signed in to change notification settings - Fork 40
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 AVIF 3-layer progressive images #134
Conversation
Thanks for the files. Overall, they look good, except for the brands. Also, we could have additional files with the layering boxes. Visually tiger_3layer_3res.avif looks great. It really shows the enhancements brought by each layer (I did not pay attention to the size overhead). Looking at the different output images for tiger_3layer_1res.avif, I don't see much differences between layer 0 and layer 1, and even layer 2 is not that great visually. Structurally, in both files, the file type box does not list |
Yeah, I was not 100% sure on the usage of a1lx, but the discussion on #131 answered my questions. As a result, I think updating these to be a single image item with a1lx makes the most sense. |
The brands are now corrected, and the images use a single Item rather than three separate ones. They do not yet have a1lx. I also stretched out the qualities on the 1res variant to make the differences more obvious. The "avif" brand issue wasn't detected by Compliance Warden, I posted an issue gpac/ComplianceWarden#19 |
I think you've found another missing test in ComplianceWarden :) Your 3res file sets the primary item id to 3 but the only item id is 1. |
2eba8e3
to
777a937
Compare
PR has been updated to reflect changes in the a1lx spec. |
b6e60cf
to
8d475e2
Compare
f7a0f6b
to
628d153
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry for the delay. Currently in the process of updating my test scripts with the new box layout so I can simulate progressive decoding of the new files. Will verify that everything works correctly by EOD tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the files!
quebec_3layer_op2.avif
seems to be using an incorrect a1op
box. From what I can see when parsing the boxes, the a1op
in the file seems to be a simple box. I can't remember if we decided to change the a1op
to be a simple box as well as the a1lx, but looking at the latest commit of the spec it looks like it's still specified as a full box.
@cconcolato do you remember?
Apart from that, would it be possible to make either fruits_2layer_thumbsize.avif
or tiger_3layer_1res.avif
use the 16-bit a1lx
element size? I'll try to make my grid files use the smaller size as well now that the last layer size doesn't need to be specified.
Thanks for the review. My recollection was that a1op was intended to be switched to a simple box as well, but I see the spec isn't changed. I can change it either way. When I update the files for this I'll also shrink a1lx to the smallest possible size (16 bit for most of these). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Added minor comments inline.
* [Layer 2, 1216x832](tiger_3layer_3res_layer2.png) | ||
|
||
* [fruits_2layer_thumbsize.avif](fruits_2layer_thumbsize.avif) | ||
2-layer progressively decodeable image, with one resolution thumbnail-sized, without operating point selection. Decoded layers are provided in: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the meeting, might be worth adding a note here if layer 1 ends up not using layer 0 for prediction due to the size difference being more than 2x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the spec again (page 260 https://aomediacodec.github.io/av1-spec/av1-spec.pdf#page=272) and the 2x limit only applies to downscaling. Here we're upscaling from the thumbnail and so the limit is 16x.
This commit adds five progressive images. These images pass the Compliance Warden validator: https://gpac.github.io/ComplianceWarden-wasm/avif.html with the exception of the still_picture and reduced_still_picture_header flags, see issue AOMediaCodec#133.
tiger_3layer_1res.avif also now uses the shorter a1lx coding. |
@tdaede @negge @leo-barnes @cconcolato Hi Thomas, Nathan, I am implementing progressive decoding of AVIF images in Chrome. I found that you and Joe Drago seem to interpret the Consider a I think Joe's interpretation matches the spec, which says:
and also:
Could you please check the Xiph three-layer test images to see if they have cumulative sizes in the Note: The layer_size array for two-layer images is Thanks, |
Good catch! If I remember the discussion correctly I also think |
Yes, they have cumulative sizes, (as does the GPAC implementation). I can correct the GPAC implementation and regenerate those test images to make them not cumulative. |
Hi Thomas, Thanks a lot! If you could generate a 4-layer progressive image, that would help improve test coverage. (I don't think anyone will use 4 layers in practice, so this would be mainly for testing.) |
This commit adds two images, one with quality scaling at the same
resolution, and one with scaling at different resolutions.
These images pass the Compliance Warden validator:
https://gpac.github.io/ComplianceWarden-wasm/avif.html
with the exception of the still_picture and
reduced_still_picture_header flags, see issue #133.