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.

Sign up for GitHub

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

Allow odd clap dimensions and offsets #2426

Merged
merged 12 commits into from
Jan 13, 2025
Merged

Allow odd clap dimensions and offsets #2426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
67025a1
Allow odd clap dimensions and offsets
y-guyon Sep 10, 2024
62b1d99
Move oddness check to CropRectRequiresUpsampling()
y-guyon Nov 22, 2024
607096e
Remove unused variable
y-guyon Nov 22, 2024
0191fd5
Inline avifCodecConfigurationBoxGetFormat()
y-guyon Nov 22, 2024
ce97e08
Apply comments
y-guyon Jan 8, 2025
8010388
Update CHANGELOG.md
y-guyon Jan 8, 2025
dfecdc4
Remove trailing space
y-guyon Jan 8, 2025
baf2525
Reword comment
y-guyon Jan 9, 2025
dead45b
Add comments about applying transforms
y-guyon Jan 10, 2025
2be929f
Replace avifCropRectConvertCleanApertureBox()
y-guyon Jan 10, 2025
0323aba
Apply to avifpng, avifjpeg and libavif_jni
y-guyon Jan 13, 2025
ee24cbb
Add avifCropRectRequiresUpsampling to CHANGELOG.md
y-guyon Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ The changes are relative to the previous release, unless the baseline is specifi
* For dependencies, the deprecated way of setting AVIF_LOCAL_* to ON is
removed. Dependency options can now only be set to OFF/LOCAL/SYSTEM.
* Change the default quality for alpha to be the same as the quality for color.
* Allow decoding subsampled images with odd Clean Aperture dimensions or offsets.
* Deprecate avifCropRectConvertCleanApertureBox() and
avifCleanApertureBoxConvertCropRect(). Replace them with
avifCropRectFromCleanApertureBox() and avifCleanApertureBoxFromCropRect().
* Add avifCropRectRequiresUpsampling().

## [1.1.1] - 2024-07-30

Expand Down
13 changes: 8 additions & 5 deletions android_jni/avifandroidjni/src/main/jni/libavif_jni.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,15 @@ bool CreateDecoderAndParse(AvifDecoderWrapper* const decoder,
avifDiagnostics diag;
// If the image does not have a valid 'clap' property, then we simply display
// the whole image.
// TODO(vigneshv): Handle the case of avifCropRectRequiresUpsampling()
// returning true.
if (!(decoder->decoder->image->transformFlags & AVIF_TRANSFORM_CLAP) ||
!avifCropRectConvertCleanApertureBox(
!avifCropRectFromCleanApertureBox(
&decoder->crop, &decoder->decoder->image->clap,
decoder->decoder->image->width, decoder->decoder->image->height,
decoder->decoder->image->yuvFormat, &diag)) {
&diag) ||
avifCropRectRequiresUpsampling(&decoder->crop,
decoder->decoder->image->yuvFormat)) {
decoder->crop.width = decoder->decoder->image->width;
decoder->crop.height = decoder->decoder->image->height;
decoder->crop.x = 0;
Expand Down Expand Up @@ -414,9 +418,8 @@ FUNC(jstring, versionString) {
libyuv_version[0] = '\0';
}
char version_string[512];
snprintf(version_string, sizeof(version_string),
"libavif: %s. Codecs: %s.%s", avifVersion(),
codec_versions, libyuv_version);
snprintf(version_string, sizeof(version_string), "libavif: %s. Codecs: %s.%s",
avifVersion(), codec_versions, libyuv_version);
return env->NewStringUTF(version_string);
}

Expand Down
12 changes: 12 additions & 0 deletions apps/avifdec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,18 @@ int main(int argc, char * argv[])
printf("Image details:\n");
avifImageDump(decoder->image, 0, 0, decoder->progressiveState);

if (decoder->image->transformFlags & AVIF_TRANSFORM_CLAP) {
avifCropRect cropRect;
if (!avifCropRectFromCleanApertureBox(&cropRect,
&decoder->image->clap,
decoder->image->width,
decoder->image->height,
&decoder->diag)) {
// Should happen only if AVIF_STRICT_CLAP_VALID is disabled.
fprintf(stderr, "Warning: Invalid Clean Aperture values\n");
}
}

if (ignoreICC && (decoder->image->icc.size > 0)) {
printf("[--ignore-icc] Discarding ICC profile.\n");
// This cannot fail.
Expand Down
11 changes: 5 additions & 6 deletions apps/avifenc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ static avifBool strpre(const char * str, const char * prefix)
return strncmp(str, prefix, strlen(prefix)) == 0;
}

static avifBool convertCropToClap(uint32_t srcW, uint32_t srcH, avifPixelFormat yuvFormat, uint32_t clapValues[8])
static avifBool convertCropToClap(uint32_t srcW, uint32_t srcH, uint32_t clapValues[8])
{
avifCleanApertureBox clap;
avifCropRect cropRect;
Expand All @@ -399,13 +399,12 @@ static avifBool convertCropToClap(uint32_t srcW, uint32_t srcH, avifPixelFormat

avifDiagnostics diag;
avifDiagnosticsClearError(&diag);
avifBool convertResult = avifCleanApertureBoxConvertCropRect(&clap, &cropRect, srcW, srcH, yuvFormat, &diag);
avifBool convertResult = avifCleanApertureBoxFromCropRect(&clap, &cropRect, srcW, srcH, &diag);
if (!convertResult) {
fprintf(stderr,
"ERROR: Impossible crop rect: imageSize:[%ux%u], pixelFormat:%s, cropRect:[%u,%u, %ux%u] - %s\n",
"ERROR: Impossible crop rect: imageSize:[%ux%u], cropRect:[%u,%u, %ux%u] - %s\n",
srcW,
srcH,
avifPixelFormatToString(yuvFormat),
cropRect.x,
cropRect.y,
cropRect.width,
Expand Down Expand Up @@ -2298,7 +2297,7 @@ int main(int argc, char * argv[])
image->pasp.vSpacing = settings.paspValues[1];
}
if (cropConversionRequired) {
if (!convertCropToClap(image->width, image->height, image->yuvFormat, settings.clapValues)) {
if (!convertCropToClap(image->width, image->height, settings.clapValues)) {
goto cleanup;
}
settings.clapValid = AVIF_TRUE;
Expand All @@ -2318,7 +2317,7 @@ int main(int argc, char * argv[])
avifCropRect cropRect;
avifDiagnostics diag;
avifDiagnosticsClearError(&diag);
if (!avifCropRectConvertCleanApertureBox(&cropRect, &image->clap, image->width, image->height, image->yuvFormat, &diag)) {
if (!avifCropRectFromCleanApertureBox(&cropRect, &image->clap, image->width, image->height, &diag)) {
fprintf(stderr,
"ERROR: Invalid clap: width:[%d / %d], height:[%d / %d], horizOff:[%d / %d], vertOff:[%d / %d] - %s\n",
(int32_t)image->clap.widthN,
Expand Down
2 changes: 1 addition & 1 deletion apps/shared/avifjpeg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1300,7 +1300,7 @@ avifBool avifJPEGWrite(const char * outputFilename, const avifImage * avif, int
if (avif->transformFlags & AVIF_TRANSFORM_CLAP) {
avifCropRect cropRect;
avifDiagnostics diag;
if (avifCropRectConvertCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, avif->yuvFormat, &diag) &&
if (avifCropRectFromCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, &diag) &&
(cropRect.x != 0 || cropRect.y != 0 || cropRect.width != avif->width || cropRect.height != avif->height)) {
// TODO: https://github.com/AOMediaCodec/libavif/issues/2427 - Implement.
fprintf(stderr,
Expand Down
2 changes: 1 addition & 1 deletion apps/shared/avifpng.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ avifBool avifPNGWrite(const char * outputFilename, const avifImage * avif, uint3
if (avif->transformFlags & AVIF_TRANSFORM_CLAP) {
avifCropRect cropRect;
avifDiagnostics diag;
if (avifCropRectConvertCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, avif->yuvFormat, &diag) &&
if (avifCropRectFromCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, &diag) &&
(cropRect.x != 0 || cropRect.y != 0 || cropRect.width != avif->width || cropRect.height != avif->height)) {
// TODO: https://github.com/AOMediaCodec/libavif/issues/2427 - Implement.
fprintf(stderr,
Expand Down
8 changes: 4 additions & 4 deletions apps/shared/avifutil.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,14 @@ static void avifImageDumpInternal(const avifImage * avif, uint32_t gridCols, uin
avifCropRect cropRect;
avifDiagnostics diag;
avifDiagnosticsClearError(&diag);
avifBool validClap =
avifCropRectConvertCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, avif->yuvFormat, &diag);
avifBool validClap = avifCropRectFromCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, &diag);
if (validClap) {
printf(" * Valid, derived crop rect: X: %d, Y: %d, W: %d, H: %d\n",
printf(" * Valid, derived crop rect: X: %d, Y: %d, W: %d, H: %d%s\n",
cropRect.x,
cropRect.y,
cropRect.width,
cropRect.height);
cropRect.height,
avifCropRectRequiresUpsampling(&cropRect, avif->yuvFormat) ? " (upsample before cropping)" : "");
} else {
printf(" * Invalid: %s\n", diag.error);
}
Expand Down
2 changes: 1 addition & 1 deletion apps/shared/y4m.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ avifBool y4mWrite(const char * outputFilename, const avifImage * avif)
if (avif->transformFlags & AVIF_TRANSFORM_CLAP) {
avifCropRect cropRect;
avifDiagnostics diag;
if (avifCropRectConvertCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, avif->yuvFormat, &diag) &&
if (avifCropRectFromCleanApertureBox(&cropRect, &avif->clap, avif->width, avif->height, &diag) &&
y-guyon marked this conversation as resolved.
Show resolved Hide resolved
(cropRect.x != 0 || cropRect.y != 0 || cropRect.width != avif->width || cropRect.height != avif->height)) {
// TODO: https://github.com/AOMediaCodec/libavif/issues/2427 - Implement.
fprintf(stderr,
Expand Down
43 changes: 30 additions & 13 deletions include/avif/avif.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,10 @@ typedef struct avifPixelAspectRatioBox
typedef struct avifCleanApertureBox
{
// 'clap' from ISO/IEC 14496-12:2022 12.1.4.3
// Note that ISO/IEC 23000-22:2024 7.3.6.7 requires the decoded image to be upsampled to 4:4:4 before
// clean aperture is applied if a clean aperture size or offset is odd in a subsampled dimension.
y-guyon marked this conversation as resolved.
Show resolved Hide resolved
// However, AV1 supports odd dimensions with chroma subsampling in those directions, so only apply the
// requirements to offsets.

// a fractional number which defines the width of the clean aperture image
uint32_t widthN;
Expand Down Expand Up @@ -542,18 +546,27 @@ typedef struct avifCropRect

// These will return AVIF_FALSE if the resultant values violate any standards, and if so, the output
// values are not guaranteed to be complete or correct and should not be used.
AVIF_NODISCARD AVIF_API avifBool avifCropRectConvertCleanApertureBox(avifCropRect * cropRect,
const avifCleanApertureBox * clap,
uint32_t imageW,
uint32_t imageH,
avifPixelFormat yuvFormat,
avifDiagnostics * diag);
AVIF_NODISCARD AVIF_API avifBool avifCleanApertureBoxConvertCropRect(avifCleanApertureBox * clap,
const avifCropRect * cropRect,
uint32_t imageW,
uint32_t imageH,
avifPixelFormat yuvFormat,
avifDiagnostics * diag);
AVIF_NODISCARD AVIF_API avifBool avifCropRectFromCleanApertureBox(avifCropRect * cropRect,
const avifCleanApertureBox * clap,
uint32_t imageW,
uint32_t imageH,
avifDiagnostics * diag);
AVIF_NODISCARD AVIF_API avifBool avifCleanApertureBoxFromCropRect(avifCleanApertureBox * clap,
const avifCropRect * cropRect,
uint32_t imageW,
uint32_t imageH,
avifDiagnostics * diag);
// If this function returns true, the image must be upsampled from 4:2:0 or 4:2:2 to 4:4:4 before
// Clean Aperture values are applied. This can be done by converting the avifImage to RGB using
// avifImageYUVToRGB() and only using the cropRect region of the avifRGBImage.
AVIF_NODISCARD AVIF_API avifBool avifCropRectRequiresUpsampling(const avifCropRect * cropRect, avifPixelFormat yuvFormat);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Future: I think we should add an avifImageUpsample() or avifImageUpsampleTo444() function for completeness.

I also found that I did not read your reply to my suggestion of adding avifCropRectRequiresUpsampling(). Sorry about that. I sometimes review a new version of the pull request directly, without reading the comments.

You are right that libavif users may neglect to call avifCropRectRequiresUpsampling(). To help prevent that mistake, we should either return the "requires upsampling" boolean as an output parameter (as you did before), or upsample the image for the users. If we do the latter, we might as well crop the image for the users.

Copy link
Collaborator Author

@y-guyon y-guyon Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

"requires upsampling" boolean as an output parameter (as you did before)

We went away from that approach already, and it does not cover the orientation.

upsample the image for the users

I began writing a PR for that and quickly gave up. It requires a lot of changes, especially because the whole implementation revolves around avifDecoder::image. Since it is part of the public API, it must stay there, and it must be the buffer that is cropped/orientated/upsampled. However it is also used by the underlying codec, so it cannot be cropped/oriented/upsampled (at least till the last frame is decoded or the codec is destroyed).
So some internal avifDecoderData::image must be introduced, and avifDecoder->image is either a copy or a view of it, depending on the transform.
Upsampling can be done in multiple ways (nearest etc.) and has to take into account chroma citing metadata. avifDecoderDecodedRowCount() must also be adapted to cropping/orientation.

A cheaper alternative could be a helper function that returns a new image object with all the necessary transforms applied, and users must call that. But it will not go well with incremental support and may introduce unnecessary buffer copies.

Yet another path is to add a public API avifDecoder::imageWithTransformsApplied. This is roughly the same amount of work as the internal avifDecoderData::image but with fewer risks of breaking something in the existing API.

Overall it felt like a lot of work compared to just writing in avif.h that users must take care of that themselves, which I agree is not great, but also already the current situation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for the analysis.

Do you think we should provide an avifImageUpsample() function? Or should we just document that one can also convert YUV to RGB and then crop the RGB image?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think we should provide an avifImageUpsample() function?

In Chrome, samples can be rendered directly without being converted to RGB first, right? If so, it would be useful at least for this kind of use case.
Is orientation and cropping both applied in Chrome?

Or should we just document that one can also convert YUV to RGB and then crop the RGB image?

That sounds useful to mention, even if libavif offers an alternative in the future. Done.

Also added another comment about applying transform next to avifDecoder::image.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Chrome's blink::AVIFImageDecoder can provide either YUV samples or RGB samples to its callers. Cropping is done by blink::AVIFImageDecoder. (It requires that the top left corner of the clean aperture be located at (0, 0).) Orientation is applied by the callers of blink::AVIFImageDecoder.


// Deprecated. Use avifCropRectFromCleanApertureBox() instead.
AVIF_NODISCARD AVIF_API avifBool
avifCropRectConvertCleanApertureBox(avifCropRect *, const avifCleanApertureBox *, uint32_t, uint32_t, avifPixelFormat, avifDiagnostics *);
// Deprecated. Use avifCleanApertureBoxFromCropRect() instead.
AVIF_NODISCARD AVIF_API avifBool
avifCleanApertureBoxConvertCropRect(avifCleanApertureBox *, const avifCropRect *, uint32_t, uint32_t, avifPixelFormat, avifDiagnostics *);

// ---------------------------------------------------------------------------
// avifContentLightLevelInformationBox
Expand Down Expand Up @@ -1124,7 +1137,7 @@ typedef enum avifStrictFlag
AVIF_STRICT_PIXI_REQUIRED = (1 << 0),

// This demands that the values surfaced in the clap box are valid, determined by attempting to
// convert the clap box to a crop rect using avifCropRectConvertCleanApertureBox(). If this
// convert the clap box to a crop rect using avifCropRectFromCleanApertureBox(). If this
// function returns AVIF_FALSE and this strict flag is set, the decode will fail.
AVIF_STRICT_CLAP_VALID = (1 << 1),

Expand Down Expand Up @@ -1297,6 +1310,10 @@ typedef struct avifDecoder
// legal to call avifImageYUVToRGB() on this in between calls to avifDecoderNextImage(), but use
// avifImageCopy() if you want to make a complete, permanent copy of this image's YUV content or
// metadata.
//
// For each field among clap, irot and imir, if the corresponding avifTransformFlag is set, the
// transform must be applied before rendering or converting the image, or forwarded along as
// attached metadata.
avifImage * image;

// Counts and timing for the current image in an image sequence. Uninteresting for single image files.
Expand Down
Loading