-
Notifications
You must be signed in to change notification settings - Fork 195
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
Check version numbers of boxes related to the ISO gain map experimental feature #2369
Comments
|
Chris: Thank you for the reply. If I understand it correctly, this means the current implementation in libavif needs to allow unrecognized fields defined in
We should change that final return statement to Do you agree? |
Alternatively we can restrict that check to
|
Another thing to check: If |
Yes, I noticed this behavior, and I agree that it should be ignored and not fatal for the image decode. |
Thanks for looking into this Wan-Teh, I agree with your interpretation and proposed changes.
|
Hi Maryla, Since the version field comes from ISO BMFF itself, if the version of the tmap box is unknown, I think we should ignore the tmap box since tmap is not a required box. Re: the |
I agree with this. I don't think there is any use in reporting that the image has a gainmap if the library cannot do anything with it. |
Maryla, Vignesh: Chris sent me a copy of the spec (CD 21496-1). The following paragraph in the spec answers my question in #2369 (comment):
Because padding data are allowed, I think we should ignore all remaining data at the end of the |
Hi Maryla, I wrote the following answer to your first question:
Please ignore this answer. I misunderstood "the 'version' tag that is read just before 'minimum_version'". I didn't see the specification of the version tag in CD 21496-1. Where is it specified? |
The version tag is defined in ISO/IEC 23008-12:2024/AMD 1:2024(E), Clause 6.6.2.4.2:
It does NOT have padding data after the end of the
Clause 6.6.2.4.3 says:
Returning an error would also meet the "shall not process" requirement, but I think it is better to ignore the tmap and display the base image if the version is unknown. |
I sent PR #2380 but wasn't completely sure what to do with 'gainMapPresent' with regards to 'enableParsingGainMapMetadata'.
But if we don't read the metadata, we cannnot know whether that metadata is supported. |
Ignore gain maps with unsupported version. Allow extra bytes after the gain map metadata of writer_version is larger than the supported version. Issue #2369
Maryla: Regarding your question on There are two simple solutions.
Note: It is not clear why the parsing of gain map metadata and the decoding of gain map image need to be controlled separately. This is complicated and it is not clear whether all four combinations of these two boolean options are allowed. For example, is the combination |
Thanks for your input Wan-Teh, you raise very good points. The reason that there are two booleans to control the behavior is because Currently all 4 combinations are indeed allowed (and are tested in avifgainmaptest.cc)
Default value, useful for decoders that don't care about gain maps at all. In this case admittedly they probably don't care about
This is used in Chrome right now. Useful for decoders that want to extract the gain map metadata but not the gain map image itself at that time. (Chrome decodes the gain map later).
A user that previously used the
Parse metadata and decode gain map all at once. Most users that care about gain maps will probably use this. Starting from this PR (that is now merged) the behavior regarding gain map version is as follows:
I feel like this gives a lot of control but it's also complicated/overkill. I agree with Vignesh that "there is [no] use in reporting that the image has a gainmap if the library cannot do anything with it" so I don't think your solution 1 is the best. |
Maryla: Thank you very much for working out the details of all four combinations. I understand the two options now. Since the two options look progressive (i.e., the second option seems to require the first option), the third combination (first option=false, second option=true) is surprising. Does the API support providintg gain map metadata as input to avifDecoder? It would make the two options easier to understand if they are progressive. So I support disallowing the third combination. |
No because the gain map metadata is not needed nor used by the decoder. It's just provided to the user as is. The user can then use the gain map metadata in combination with the gain map image pixels (and the base image) to do tonemapping, for example by using the In most cases it would make sense to get both metadata and pixels at the same time by setting both |
Thanks for the clarification. I didn't know that the gain map metadata is not needed nor used by the decoder. That's the subtlety I was missing. |
Maryla: Re: When I wrote my previous comments on this issue, I didn't know about the requirement of having the This simple definition of
Differences from the current definition:
So we need to evaluate whether this simple definition, while easy to state and implement, is useful to our users. |
Thanks for the suggestion. Indeed this would be a simple/easy interpretation, but I think it's less useful to users. In that case we might want to have two different presence fields, making the API more complicated... |
I just realized that I may have made a mistake in my analysis. I said the simple definition of Also, it seems that I'd like to study this issue again when Chrome sets the |
@maryla-uc @vigneshvg @y-guyon @ccameron-chromium
Please audit our ISO gain map code and make sure we check the version numbers of the boxes related to ISO gain maps.
I took a look at the
avifParseToneMappedImageBox()
function in src/read.c. We checkversion
andminimum_version
but ignorewriter_version
. Should we checkwriter_version
?Is there any other box we need to check?
The text was updated successfully, but these errors were encountered: