Make decoder reset preserve generated alpha grids - #3310
Make decoder reset preserve generated alpha grids#3310carrerasdarren-cell wants to merge 1 commit into
Conversation
|
Thank you for your interest in libavif. This behavior may no longer be compliant (assuming it was so far): #1203 (comment) |
|
Thanks for flagging this. This PR only makes repeated decoder resets consistent with libavif's existing synthesized-alpha-grid behavior; the generated item is internal and the patch does not introduce a new association in the input file. The cited draft does make the premise of that compatibility path questionable. If libavif removes support for carrying tile alpha into a derived color grid, this patch and its reset regression should not land as-is. I suggest holding #3310 until the direction in #1203 is decided. If the behavior remains as an optional compatibility path, the reset failure still needs this kind of fix and I can adjust the patch to the preferred design. |
| // Adopt the configuration property of the first image item tile, so that it can be queried from | ||
| // the top-level color/alpha item during avifDecoderReset(). | ||
| const avifCodecType codecType = avifGetCodecType(item->type); | ||
| const char * configPropName = avifGetConfigurationPropertyName(codecType); | ||
| const avifProperty * srcProp = avifPropertyArrayFind(&item->properties, configPropName); | ||
| if (!srcProp) { | ||
| avifDiagnosticsPrintf(&decoder->diag, "Grid image's first tile is missing an %s property", configPropName); | ||
| return AVIF_RESULT_INVALID_IMAGE_GRID; | ||
| } | ||
| avifProperty * dstProp = (avifProperty *)avifArrayPush(&gridItem->properties); | ||
| AVIF_CHECKERR(dstProp != NULL, AVIF_RESULT_OUT_OF_MEMORY); | ||
| *dstProp = *srcProp; | ||
| if (!avifPropertyArrayFind(&gridItem->properties, configPropName)) { | ||
| avifProperty * dstProp = (avifProperty *)avifArrayPush(&gridItem->properties); | ||
| AVIF_CHECKERR(dstProp != NULL, AVIF_RESULT_OUT_OF_MEMORY); | ||
| *dstProp = *srcProp; | ||
| } |
There was a problem hiding this comment.
This change is unrelated to alpha.
If I understand correctly, at head every consecutive call to avifDecoderReset() will allocate a new property if the input file contains a grid. This is why a check is added here to avoid adding the property if it already exists. Is this correct?
This is a bit late in the libavif development process, but maybe avifDecoderReset() should not create "fake" properties. That should be done in avifDecoderParse() only in my opinion.
I sent #3311 to avoid having to create that fake property at all. That does not fix the other issue about alpha.
There was a problem hiding this comment.
Yes, that is correct: the added check prevents each avifDecoderReset() call from appending another copied codec property. I agree that #3311 removes the need for that part of this patch entirely. I will keep #3310 on hold pending the direction in #1203; if the compatibility behavior remains, I will rebase on #3311 and retain only the generated-alpha identity/reset handling and its regression test.
2 participants
avifMetaFindAlphaItem()synthesizes an alpha grid when a color grid'sindividual tiles each have alpha auxiliaries. The first reset rewrites those
alpha tile
dimgForIDassociations to the generated grid. A lateravifDecoderReset()then mistakes the rewritten associations for malformedinput and returns
AVIF_RESULT_INVALID_IMAGE_GRID.Record and reuse the generated alpha grid on later resets, and avoid appending
duplicate adopted codec configuration properties to grid items. Extend the
existing
color_grid_alpha_nogrid.avifregression test to decode successfullyacross three passes with two resets.
Tests:
avifdecodetestwith UndefinedBehaviorSanitizer: 8/8 passed