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

Change avifParsePixelInformationProperty() to ignore and skip pixi boxes with version > 0 #2428

Open
wantehchang opened this issue Sep 11, 2024 · 3 comments

Comments

@wantehchang
Copy link
Collaborator

@leo-barnes @podborski @y-guyon

If we see a pixi box with version > 0, we should ignore and skip the box. Right now libavif fails with the AVIF_RESULT_BMFF_PARSE_FAILED error if it sees a pix box with version > 0:

static avifBool avifParsePixelInformationProperty(avifProperty * prop, const uint8_t * raw, size_t rawLen, avifDiagnostics * diag)
{           
    BEGIN_STREAM(s, raw, rawLen, diag, "Box[pixi]");
    AVIF_CHECK(avifROStreamReadAndEnforceVersion(&s, 0));
    ...

and

        } else if (!memcmp(header.type, "pixi", 4)) {
            AVIF_CHECKERR(avifParsePixelInformationProperty(prop, avifROStreamCurrent(&s), header.size, diag),
                          AVIF_RESULT_BMFF_PARSE_FAILED);

We can fix this by changing avifParsePixelInformationProperty() to return avifResult and having it return AVIF_RESULT_NOT_IMPLEMENTED if version > 0, and changing the caller to ignore the AVIF_RESULT_NOT_IMPLEMENTED error.

Note: I think the caller need to call avifArrayPop(properties) when avifParsePixelInformationProperty() returns AVIF_RESULT_NOT_IMPLEMENTED to clean up properly.

@y-guyon
Copy link
Collaborator

y-guyon commented Sep 12, 2024

Is it allowed to have multiple properties of the same type for the same item? For example one could have a pixi version 0 for backward compatibility and another pixi version 1 for the extended fields but with the same values as the other pixi for the fields in common.

Otherwise pixi version 1 will never be backward compatible because it is mandatory, right?

@leo-barnes
Copy link

@y-guyon

Is it allowed to have multiple properties of the same type for the same item?

It depends on the property. 'pixi' says this:

Box type: 'pixi'
Property type: Descriptive item property
Container: ItemPropertyContainerBox
Mandatory (per item): No
Quantity (per item): At most one

So we can't have one with version 0 and one with version 1, unless we change the text say "At most one per version". But that might cause issues for older parsers as well.

Given the discussion in the meeting yesterday and the thread with Dirk I think we should not bump the version and simply extend it.

@leo-barnes
Copy link

This issue is really part of a larger issue. Ideally all non-essential item properties that not understood (or have an unsupported version) should be ignored.

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

No branches or pull requests

3 participants