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

Please add some AVIF testfiles with matrix_coefficients == 0 #195

Closed
novomesk opened this issue Sep 14, 2022 · 17 comments
Closed

Please add some AVIF testfiles with matrix_coefficients == 0 #195

novomesk opened this issue Sep 14, 2022 · 17 comments

Comments

@novomesk
Copy link

I think it would be useful to have testfiles with zero MatrixCoefficients, so that various AVIF implementations can test before release.

For example following file is rendered differently in various applications, because they perform unnecessary YUV->RGB conversion:
avif-rgb-8bit.zip

Safari on iPhone (iOS 16.0):
iOS16-Safari

AV1 Extension on Windows:
BTW, there is some weird stripe on the right side.
Microsoft-AV1_Video_Extension

It looks OK in the following apps (background color may be different but that's expected):

Opera browser:
Opera

qimgv viewer:
qimgv

GIMP:
GIMP

@leo-barnes
Copy link
Collaborator

leo-barnes commented Sep 14, 2022

There is actually already test content using identity matrix in the repo. Here is one example:
https://github.com/AOMediaCodec/av1-avif/blob/master/testFiles/Netflix/avif/hdr_cosmos01000_cicp9-16-0_lossless.avif

The reason why the file you attached renders differently in various places is because it's actually kind of ambiguous. It contains an ICC profile but no nclx color box. ICC profiles don't actually specify the YCbCr matrix. It looks like some implementations are peeking into the actual elementary stream to figure out what the matrix should be.

The latest wording in MIAF attempts to clear this up, but I don't think that version has been published yet. The gist of it is:

  1. You can now have both an ICC profile and an NCLX color box. ICC profile has precedence. This means you can use the ICC profile as usual, but use the NCLX box to specify full/video-range and YCbCr matrix.
  2. If there is no color box, it defaults to primaries 1, transfer characteristics 13, matrix coefficients 5 or 6 (they are the same), full range 1.

There is also a note saying:

Any colour information in the bitstream is ignored by the MIAF reader and MIAF renderer processing models. The colour information property whether explicit or default, takes precedence over any colour information in the image bitstream.

I still think the wording is slightly unclear on what should happen if you only have an ICC profile. @cconcolato, can you remember what the decision was? Since the ICC profile does not override the default matrix coefficients, should the default be used rather than what might be in the elementary stream?

@novomesk
Copy link
Author

The colour information property whether explicit or default, takes precedence over any colour information in the image bitstream.

The clarification would be useful.
Because I understand that "takes precedence" doesn't mean to ignore completely the info from bitstream.

@leo-barnes
Copy link
Collaborator

The clarification would be useful. Because I understand that "takes precedence" doesn't mean to ignore completely the info from bitstream.

Well, it does actually explicitly say Any colour information in the bitstream is ignored by the MIAF reader. A strict interpretation of that would be that if YCbCr matrix is not specified in an nclx box (or potentially in the av1C in the optional config OBUs), the default value shall be used.

I still think the spec could spell out exactly what is meant to happen when you only have an ICC profile just to make it abundantly clear what the expectation is.

@novomesk
Copy link
Author

It means that files with ICC only will be handled like matrix coefficients 5,6

But it will break lot of older files which use coefficients 1 in the bitstream. MC == 0 is not so frequent but MC == 1 is used. Those old files will be decoded with slightly different colors, people will not be happy that.

@baumanj
Copy link
Collaborator

baumanj commented Sep 15, 2022

Per @cconcolato 's comment here, if there is no colr box with colour_type 'nclx', then if the AV1 bitstream has color_description_present set in the sequence header and the value of MC isn't 2 (unspecified), then the MC value from the AV1 bitstream is used.

I believe that's what was agreed to in #84 and is what I implemented in Firefox. I think the files you're referring to with ICC embedded in a colr box and MC == 1 in the AV1 bitstream should be handled correctly, @novomesk. That said, I'm not sure there are any test files like that in this repo, so maybe it would be good to add some.

@leo-barnes
Copy link
Collaborator

Per @cconcolato 's comment here, if there is no colr box with colour_type 'nclx', then if the AV1 bitstream has color_description_present set in the sequence header and the value of MC isn't 2 (unspecified), then the MC value from the AV1 bitstream is used.

That's not what the closing statement from @cconcolato says though:

The MIAF group discussed this issue and resolved to say in the upcoming amendment text that:

  • the color property (whether defaulted or explicit) always takes precedence over the bitstream value
  • when it's defaulted it means 1/13/5 (or 6)/1. RGB requires an explicit color box (not a possible default anymore, resolving this issue).
  • we still have to discuss what 2/2/2 means if used in the colour property.

@baumanj

I believe that's what was agreed to in #84 and is what I implemented in Firefox. I think the files you're referring to with ICC embedded in a colr box and MC == 1 in the AV1 bitstream should be handled correctly, @novomesk. That said, I'm not sure there are any test files like that in this repo, so maybe it would be good to add some.

Hmm... I'm not completely sure that's what we agreed upon in the meeting, but I'll try to dig up the meeting notes to check.
It's not the text that is going into the as-of-yet unpublished version of MIAF at least (it explicitly tells you that the bitstream shall be ignored).

HEIF and especially MIAF has from the start been trying to make sure that you should never need to reach into the video bitstream itself to figure out color stuff since that is not codec agnostic. For HEVC, it was considered ok (but not mandated) to check in the codec config box, since that is not part of the bitstream and the codec config is defined in ISOBMFF. But with AV1, this information is usually not present in the codec config (most likely since it can be described with a codec agnostic colr box). Unfortunately the AV1 ISOBMFF bindings did not mandate that a colr box is mandatory if the info is not present in the codec config.

So the exact behaviour for AVIF files with only an ICC profile with the old wording in MIAF is actually undefined. Which @novomesk actually noted here: #84 (comment)
The new wording in the MIAF spec tries to fix this by defining the expected behaviour. Which to my eyes is to always ignore what is in the bitstream.

So the example file here is relying on undefined behaviour. Like in C, anything might happen when you do that. :)

@novomesk
Copy link
Author

Writing two boxes (ICC+NCLX) helps in iOS 16.0 to get expected rendering. It doesn't help for the Windows extension yet.

I also observed that AVIF with 12bit depth is not displayed on iOS 16.0.

I think web developers need to know what AVIF features they can use (or what to avoid) so that their files work across the browsers. Android 12 used to support less AVIF files than Google Chrome on the same phone.

@baumanj
Copy link
Collaborator

baumanj commented Sep 16, 2022

HEIF and especially MIAF has from the start been trying to make sure that you should never need to reach into the video bitstream itself to figure out color stuff since that is not codec agnostic.

Why is that? I agree it's more convenient and consistent to get it from the colr box(es) and that it is potentially useful for overriding a bitstream with incorrect CICP values, but in the case that there's only an ICC profile in the colr box and the bitstream MC explicitly differs from the default, it seems more likely that using the default will result in incorrect rendering relative to the writer's intent.

Why should we try so hard to avoid getting this data from the bitstream? To me, it seems most likely to be accurate to how the image was encoded, and by the time a renderer is applying the MC, the bitstream has already been decoded, right? I'm all for codec agnosticism, but something at this point has to be aware of the codec type just to call the right decode function.

I could get behind the idea that this case is too ambiguous and that it's a "shall not", so readers are encouraged to reject the input, but I find it highly unlikely that we're going to see this handled consistently in practice if the standard says to ignore the bitstream's explicit signal in favor of the implied default from the container.

As for putting it in the codec config, do you mean in the configOBUs of the av1c box ? I don't think I've ever seen that. While technically possible, that seems to make the situation even more confusing since now there would be 3 places (4 with defaults) to derive the information. Where does it fall in terms of precedence?

@baumanj
Copy link
Collaborator

baumanj commented Sep 16, 2022

I think web developers need to know what AVIF features they can use (or what to avoid) so that their files work across the browsers. Android 12 used to support less AVIF files than Google Chrome on the same phone.

Given the incredibly wide variety of features possible with HEIF, it may be interesting to devise some sort of syntax to communicate this information via additional parameters on the MIME type, right? That should probably be a whole separate issue/discussion, though.

@leo-barnes
Copy link
Collaborator

@baumanj

Why is that? I agree it's more convenient and consistent to get it from the colr box(es) and that it is potentially useful for overriding a bitstream with incorrect CICP values, but in the case that there's only an ICC profile in the colr box and the bitstream MC explicitly differs from the default, it seems more likely that using the default will result in incorrect rendering relative to the writer's intent.
Why should we try so hard to avoid getting this data from the bitstream? To me, it seems most likely to be accurate to how the image was encoded, and by the time a renderer is applying the MC, the bitstream has already been decoded, right? I'm all for codec agnosticism, but something at this point has to be aware of the codec type just to call the right decode function.

YCbCr <-> RGB conversion is typically not handled by the codec but rather at the container level, in the same way that color primaries and transfer function typically is not really applied or used by the codec. I would guess the reason it's possible to signal it at the codec level is twofold:

  • The matrix coefficients and/or transfer function affects statistics on the input and serves as a hint to the encoder on how to most efficiently compress the content.
  • There probably exists some very lightweight container that does not specify this information by itself.

Given that this is happening at the container level rather than at the codec level, I would argue that it's actually more likely that the container level has the correct values and that whatever is in the bitstream is incorrect. But all of this is actually kind of beside the point. :)


It would be fine to check the bitstream for CICP if the spec mandated that this is the required behaviour. But the spec doesn't (or rather didn't) say anything about it at all, which means the behaviour is undefined. If we want to make the behaviour defined, we have two choices:

  1. Mandate that matrix coefficients and/or full/video-range settings shall be extracted from bitstream if missing
  2. Mandate that CICP and/or full/video-range settings shall be inferred from defaults if missing

NOTE:
Notice that I do not include extracting full CICP from bitstream in option 1. The MIAF spec has mandated from the start that if there is no colr box of any kind, sRGB + full-range is the default and trumps whatever is in the bitstream. Any parser that does not do this is not compliant with either the old or the new wording of the spec, since this was was fully defined.

Option 1 may sound perfectly reasonable for a simple single-item file. But as soon as you introduce a simple grid item you have problems.

MIAF added restrictions on grid items to allow easier and more performant parsing and rendering. The idea was to make sure that decoding could be done into a single canvas buffer of the native format of the bitstream. Why is this important?

  • Performance. Many image processing pipelines can deal with 420 content directly and can bake in pixel format conversions into other operations like the final view compositing. So you get fewer operations needed, and typically get HW/GPU optimizations when doing them.
  • Reduced memory usage. 8-bit 420 uses 1.5 bytes per pixel, while RGB usese 3 (and RGBA uses 4). The longer you can keep buffers in the native format in the pipeline, the more memory you can typically save.

So how does MIAF add restrictions to make sure that all tiles can be decoded into a shared canvas?

  • Codec configuration shall be the same for all tiles in a grid
  • Color properties shall be the same for all tiles in a grid

For all other codecs than AV1, this is enough. But the AV1 codec config does not specify matrix coefficients or full/video-range. So I can create a file like the following:

  • item 0: grid that points to item 1 and item 2
  • item 1 and 2: shares av1C box and colr-ICC box. No colr-NCLX box is present.
  • item 1 bitstream says video range and 709 matrix
  • item 2 bitstream says full range and 601 matrix

Given that items 1 and 2 share av1C, we can be guaranteed that they have the same subsampling and bit-depth. We can also know that they have the same color primaries and transfer function, or at least that the container overrides whatever is in the elementary stream by providing an ICC profile.

How do we know that they have the same matrix coefficients and range? We don't.

We can't decode into a shared canvas unless we first check the bitstreams for all the tiles. This means we can't do streamed decoding (typically pretty important on the web) since we need to wait until we have the final tile.

Things only get more complicated as you start to create more complex files. Hence why I filed this issue: #194

How does this compare with mandating that the container level explicitly or implicitly overrides whatever is in the bitstream?
Once you have the meta box, you know everything you need to know about the file.

As for putting it in the codec config, do you mean in the configOBUs of the av1c box ? I don't think I've ever seen that. While technically possible, that seems to make the situation even more confusing since now there would be 3 places (4 with defaults) to derive the information. Where does it fall in terms of precedence?

And you probably won't either. :)
IIRC the ISOBMFF-AV1 bindings say that the optional config OBUs should (or maybe even shall?) not be used for stills. So it's not really possible to put it in the codec config. But since the ISOBMFF-AV1 bindings don't mandate that a colr box is required, we then basically have undefined behaviour.

@leo-barnes
Copy link
Collaborator

We can of course play devil's advocate and say that the number of files out there that contain a grid of mixed matrix coefficients or full/video-range are most likely 0, and that the number of files out there that contain an ICC profile and non-601 matrix coefficients is sizeable and we should change the spec to take this into account since the behaviour was undefined.

I'm fine with that, but it then needs to actually be specified in the spec that this is the required behaviour. Which needs the following to happen:

  1. MIAF wording is changed to allow AVIF to query the bitstream for the case where we have an colr-ICC profile but no colr-NCLX.
  2. AVIF spec is changed to solve When can sub-items share codec configuration? #194. Maybe AVIF can require that items that share av1C shall have identical Sequence Header OBU or something like that.
  3. AVIF spec (or potentially ISOBMFF-AV1 bindings?) is changed to indicate that if color properties are only partially specified (but not completely unspecified since the default then holds) the missing pieces should be retrieved from the bitstream.

@novomesk
Copy link
Author

@leo-barnes When you have time, do you know why this animation doesn't work in Safari on my iPhone8 with iOS 16.1?
animation2.zip

I saw some news that animated AVIF should be supported.

@leo-barnes
Copy link
Collaborator

leo-barnes commented Oct 27, 2022

@leo-barnes When you have time, do you know why this animation doesn't work in Safari on my iPhone8 with iOS 16.1? animation2.zip

I saw some news that animated AVIF should be supported.

@novomesk
The WebKit folks said it's due to this:
https://commits.webkit.org/253002@main

For iOS it will land in a coming update. For macOS, it is already fixed in Safari Tech Preview -- release 156.

@novomesk
Copy link
Author

Thanks! So we will wait till the next update.

@Xredmi

This comment was marked as off-topic.

1 similar comment
@Xredmi

This comment was marked as off-topic.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Nov 25, 2022
Overview of Changes from GIMP 2.99.12 to GIMP 2.99.14
=====================================================

Core:

  - The download button in About dialog when a new version is available
    will now show the development download page when running unstable
    branch code.
  - The update check on macOS now uses native HTTPS-able API, so that we
    don't have to wait for GIO to have HTTPS modules for macOS.
  - The main process is now run as a GimpApp which is a new class derived from
    GtkApplication. The main process of `gimp-console` on the other hand is a
    `GimpConsoleApp` which is derived from GApplication. Both new classes share
    a same GimpCoreApp interface. This is a main step for the GTK+3 port.
  - Various improvements on awareness of multi-item selection across core
    features. A notable fix is the preview when transforming multiple layers at
    once (with various transform tools). Various actions are now multi-drawable
    aware as well.
  - New "Vectors Structure" in the XCF format: XCF files (format bumped to
    version 18) can now store and load all the usual and common properties of
    other items. In other words, it makes XCF now able to store locks, color
    tags and several selected paths.
  - XCF saving with RLE and zlib encoding are now multi-threaded and therefore
    much faster in most cases.
  - Pasting an image now creates a new layer by default (not a Floating Layer
    anymore). The only 3 cases where we still have floating items are:
    * when pasting into a layer mask;
    * when doing quick copy/cut paste on-canvas with the Alt modifiers;
    * when floating layers explicitly with the "Float" action.
  - Copy-paste code was deeply reviewed and re-specified in the light of
    multi-item selection; it's still a WIP:
    * When pasting several drawables, we currently paste them over the top
      selected layer (visually in Layers dockable).
    * Pasted data position was rewritten, based on existing logic, but taking
      into account the multiple selected items.
    * Pasting a selected area from multiple layers still creates multiple
      layers, not merged pixel contents as a single layer.
    * New layers created when copying from a selection are consistently the
      offset and dimensions of the bounding box of the dimension.
    * When a layer and one of its group layer parent are selected, it is
      equivalent to have only the child layer selected.
  - 2 new actions were added: "Paste as Single Layer" and "Paste as Single Layer
    in Place" under the "Paste as" submenu of Edit menu. These paste the copied
    layers as a single merged layer, instead of as several layers (as "Paste"
    and "Paste in Place" do).

Graphical User Interface:

  - New "Gray" theme based on a 18.42% luminance middle-gray background, which
    should be a good neutral environment for color work.
  - The foreground/background editor in the toolbox will now take into account
    the toolbox icon size and resize itself accordingly (live, as you change
    theme). This allows to have really narrow toolbox when you use small icons.
  - Theme-override icon size selection in Preferences > Themes: this allows to
    override theme-set icon sizes, with a global concept of small, medium, large
    and huge. The following widgets are so far modified: toolbox icons, fg/bg
    editor in toolbox, fg/bg editor in Colors dockable, dockables tab icons,
    bottom buttons (in the button box) of dockables, header eye and lock icons
    above item trees, and eye and lock icon switches in item tree cells.
  - Symmetry dockable contents is now shown, yet deactivated, when no images are
    opened, improving discoverability.
  - Reworked the "Convert to * Working Space?" dialog into a "Keep the Embedded
    Working Space?" one. Keeping an image working space is now the recommended
    and default action. "Convert" became an explicit action requiring to click
    (neither mapped to Enter nor Escape keys).
  - "Floating Selection" renamed to "Floating Layer" or "Floating Mask"
    depending on the type of item it applies to.
  - "Floating Masks" are now drawn above the layer mask in the Layers dockable,
    making the fact that they would anchor to the below layer mask (not the
    layer) much more obvious.
  - "Paste into Selection" and "Paste into Selection in Place" were moved under
    the "Paste as" submenu of Edit menu.

Tools:

  - Text tool: new "Outlined" and "Outlined and filled" options, with various
    sub-options to choose the outline style, color, pattern, width, cap and join
    styles, miter limit, anti-aliasing and dash pattern.
  - Align tool:
    * now multi-item aware, it is much more usable than it used to be when we
      had to click on canvas to select items.
    * On-canvas clicks are now only needed to select guides (Alt or Alt-Shift
      click and selected guide colors change) or for the reference object
      (normal click).
    * Also the reference object gets on-canvas handles and the name is written
      in the dockable, making it obvious if you selected the right reference or
      not.
    * Moreover the selected reference will now loop when layers are stacked on
      each other, which allow to select a bottom layer, even if there are layers
      above it everywhere.
    * New option "Use extents of layer contents" to Align tool: this is similar
      to first run "Crop to Content" on every layer to align or distribute
      (without actually cropping the layers).
    * Fine-grained align/distribute button sensitivity to make it more obvious
      when an action would not make any change anyway.
    * New anchor point setting (pivot widget) to choose which part of the target
      items will be aligned or distributed.
    * Get rid of various broken distribution actions.
    * Distribution actions don't move the 2 extreme (top/bottom or left/right
      depending on distribution direction) targets, but distribute all other
      targets within their range. It is more consistent with how it works in
      other software.
    * Adding 2 "Distribute with evenly (horizontal|vertical) gaps" actions,
      which distribute by keeping a common gap between objects instead of
      between anchor points.
    * Offset settings have been removed.
  - Transform tools are now auto-activated on selection (and when switching
    images or item selection).

Plug-ins:

  - PDF:
    * Export code was ported to GimpProcedureDialog.
    * New "root-layers-only" argument to "file-pdf-save", which comes
      with a checkbox in the export dialog to allow exporting as pages
      the root layers only. The main usage is to organize your pages'
      contents in layer groups.
  - AVIF:
    * RGB AVIF compatibility with Safari on iOS 16.0: Some AVIF images are
      rendered differently in Apple's implementation compared to implementations
      of Google and Mozilla. See: AOMediaCodec/av1-avif#195
      This changes requires libheif 1.10.0 though the plug-in can still build
      with older libheif.
  - PSD:
    * export of CMYK(A) files added, with 8 or 16-bit precision per channel,
      using a CMYK soft-proof profile for conversion.
    * Paths are now exported with PSD files.
  - JPEG-XL:
    * Metadata import/export now supported (requires libjxl 0.7.0).
  - Python-Console:
    * sys.stdout.flush() implemented as a no-op inside the console, to be able
      to easily copy-paste code, or using libraries which flush the output.
  - ICNS:
    * Initial support for loading and exporting.
  - TIFF:
    * New toggle to optionally load reduced pages. We keep a heuristic to try
      and guess whether these are thumbnails (single reduced image in the second
      position), but it's only used to decide whether the option is checked by
      default or not. It is now up to anyone to decide or not whether they want
      to load these reduced images.

API:

  - Changes in libgimp:
    * Abstract method get_window() of GimpProgressVtable had its signature
      changed. The window ID is now a guint64.
    * New functions:
      + gimp_text_layer_set_markup()
      + gimp_image_get_selected_channels()
      + gimp_image_get_selected_vectors()
      + gimp_image_list_selected_channels()
      + gimp_image_list_selected_vectors()
      + gimp_image_set_selected_channels()
      + gimp_image_set_selected_vectors()
      + gimp_image_take_selected_channels()
      + gimp_image_take_selected_vectors()
      + gimp_image_list_selected_drawables()
    * Updated functions:
      + gimp_vectors_stroke_translate() now uses offsets in double type.
    * New classes:
      + GimpTextLayer: child class of GimpLayer.
  - Changes in libgimpwidgets:
    * Updated widgets:
      + GimpPickButton now has a specific implementation for Windows. In
        particular it improves color picking with multi-monitor and scales
        different than 100%.

Build:

  - meson requirement bump to meson 0.56.0.
  - Many fixes to the meson build scripts, making it closer to be our
    official build for GIMP 3.0.
  - The CI now generates a tarball containing the GIMP references,
    generated by gi-docgen and g-ir-doc.
  - Improved Clang 15.0.0 support.
  - "win*-nightly" jobs were added back and are now more efficient with the
    --output-dll-list option.
  - babl requirement bumped to babl 0.1.98.
  - GEGL requirement bumped to GEGL 0.4.40.
  - GIMP macOS builds (gimp-macos-build repository) was moved to using MacPorts
    in order to take advantage of a bigger community to maintain our
    dependencies.
  - GIMP now has an Apple Silicon build.
webkit-early-warning-system pushed a commit to shallawa/WebKit that referenced this issue Dec 12, 2022
…is a consistent image failure

https://bugs.webkit.org/show_bug.cgi?id=248544
rdar://102823196

Reviewed by Simon Fraser.

libavif does not render fast/images/resources/green-313x313.avif and
fast/images/resources/green-400x400.avif correctly.

See AOMediaCodec/av1-avif#195.

We need to fix the expectation file fast/images/avif-as-image-expected.html
with the correct rendering; i.e. the image should be lime not green. This will
require marking this test [ ImageOnlyFailure ] for non Apple ports and macOS
down-level ports.

* LayoutTests/TestExpectations:
* LayoutTests/fast/images/avif-as-image-expected.html:
* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac-ventura/TestExpectations:
* LayoutTests/platform/mac/TestExpectations:

Canonical link: https://commits.webkit.org/257738@main
@leo-barnes
Copy link
Collaborator

This issue asks for test content that uses identity matrix. Such a file already exists:
https://github.com/AOMediaCodec/av1-avif/blob/master/testFiles/Netflix/avif/hdr_cosmos01000_cicp9-16-0_lossless.avif

I think this issue can be closed. Please reopen if you think this issue covers something that I missed!

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

4 participants