Fix final review comments from Leo #256

leo-barnes · 2024-10-09T09:45:04Z

index.bs

leo-barnes · 2024-10-09T09:50:27Z

index.bs

@@ -817,12 +823,12 @@ A file containing a <code>'[=pict=]'</code> track compliant with this profile is
 <h2 id="box-lists">Box requirements</h2>


baseline-profile section example calls out that if all frames are sync samples the avio brand should be present. Do we want the same to be stated in the advanced-profile example text?

leo-barnes · 2024-10-09T09:51:46Z

index.bs

-    - <code>'[=colr=]'</code>
-    - <code>'[=pixi=]'</code>
+In addition to the Image Properties defined in this document, [=AV1 image items=] may also be associated with item properties defined in other specifications such as [[!HEIF]] and [[!MIAF]]. Examples of commonly used item properties are:
+    - <code>'[=colr=]'</code> (<dfn noexport for="AVIF">ColourInformationBox</dfn>)


I have only added the box names for those boxes where we explicitly state them in our text so we have something to link to. Do you think it looks too inconsistent to only have it for some of the properties?

Why not just refer to ISOBMFF or HEIF directly?

Good question! This kind of goes hand in hand with the newly added grid derived item section and really with the table of boxes and this whole section.

My thinking was as follows. The two main complaints about the AVIF spec I have heard over the years have been:

It's very hard to figure out how the various specs fit together.

It's unclear which things in HEIF and MIAF are actually supported/allowed in AVIF.

AVIF technically did not need to add a section on the tmap item. But we did it so it was clear that it's actually supported in AVIF. Given that we thought it was important to highlight tmap, I think it's equally important to highlight that grids are also supported (we added the notes that highlight this specifically since the JPEG-XL proponents completely missed this fact after all.)

In general, once we have a section/paragraph describing using a HEIF feature in AVIF, I have tried linking all other references in AVIF to that section and then letting that section link to HEIF. That way any additional requirements/constraints added by AVIF are not missed. (For example grpl, altr, ster, tmap and so on.)

Specifically for these boxes, we talk about colr and pixi in a bunch of places. But we never call out that colr == ColourInformationBox, and I thought it made sense to have that equivalence stated in AVIF so it's not missed. I don't have very strong feelings on colr == ColourInformationBox though so if you don't think it adds any value I'm happy to remove it. (I think we should keep grid derived item though.)

I feel it adds indirection. When one wants to look ColourInformationBox up, it will first lead to a place in AVIF, which itself redirects to HEIF. I get that this behavior is wanted to map colr to ColourInformationBox, but it also introduces confusion about which spec defines what.

It's very hard to figure out how the various specs fit together.

I do not see an easy solution to this.

It's unclear which things in HEIF and MIAF are actually supported/allowed in AVIF.

Solutions I can think of:

Rely on the fact that everything that is not explicitly forbidden is allowed (~ current situation, could be written somewhere though)

Succinctly list all boxes or features that are allowed (kind of an extended section 9. Box requirements) and declare all others as forbidden

Create a new section for each supported box or feature, as was done for tmap and now grid etc. This feels like copy/pasting parts of ISOBMFF/HEIF/MIAF, but may be the clearest way for someone that just reads AV1-AVIF and not the inherited specs.

Example for iden which is allowed by AV1-AVIF but barely supported in implementations:

Not explicitly forbidden hence allowed,

Could be listed as <code>[=iden=]</code> and that's it,

There could be a whole section about it.

Not sure which one is better than the others to be honest.

Anyway this seems beyond the topic of AVIF/ColourInformationBox and AVIF/ColourInformationBox barely answers that lack of clarity in my opinion.

I do not mind using both terms directly linked to 14496-12, especially if both are next to each other in the dfn list, but what about sticking to one of colr and ColourInformationBox if that is too much?

Not sure which one is better than the others to be honest.

Yeah, that's what I was grappling with as well.

I do not mind using both terms directly linked to 14496-12, especially if both are next to each other in the dfn list, but what about sticking to one of colr and ColourInformationBox if that is too much?

In order to improve consistency, how about this:

We switch all of the current usage of the full HEIF box names in AVIF to use the box types. That's mostly what we are using in any case, so it will only be a few changes. All of the box types that don't have explicit AVIF requirements or text links directly to the spec that defines it.

We add to the AVIF spec the boxes which are reasonably widely supported or something that we believe may end up being common in the foreseeable future. This means:

For derived images we keep the sections for grid and tmap since they are widely supported. Since iden and iovl is only supported on Apple systems (I think?) it doesn't count as widely supported so we don't add sections for those. All use of grid and tmap inside of AVIF link to the AVIF sections which in turn links to HEIF.

For item properties we add the properties that are widely supported or at least likely to be used to the list of boxes in avif-required-boxes-additional.

We either add common properties to the section "Other Item Properties" or completely remove that section and point it to avif-required-boxes-additional. I'm leaning towards the latter since it feels like we're duplicating stuff.

If we all agree on that, I guess the question comes down to which boxes to keeps in the list of boxes.

I added the grpl, altr and ster boxes since the first two are needed for tmap items and ster is pretty much the base requirement for stereo photos and we use them extensively for HEIC.

I added all of the HDR item properties that have been added since they seem like they will likely be used now or in the future (and they are used in SlimHEIF).

I also added the camera extrinsics and intrinsics since those are very important for stereo images and supported by libheif and the Apple platforms.

So colr everywhere and no mention of ColourInformationBox. This is fine with me, although I would have picked the latter, personally.

Yeah, the full box names are more descriptive. But it is kind of inconsistent to use the 4CCs for everything else, so if we use ColourInformationBox we should really switch all the others as well.

Yes, I meant switching all, colr was just an example.

Actually, looking through the doc, we use the box names consistently in some cases (FileTypeBox). Let me handle this as follows instead:
For the cases where we mostly refer to the box by the 4CC but at some point use the name, make it look like this:

<code>[=MasteringDisplayColourVolumeBox=]</code> (<code>'[=mdcv=]'</code>)

@y-guyon
Let me know what you think. I've now made everything link directly to the base spec, except in these places:

clap in table of boxes links to #clean-aperture-property section

grpl in table of boxes links to #groups section

altr in table of boxes lins to #altr-group section

ster in table of boxes lins to #ster-group section

The places where we mention using altr groups now say something like this:

should be grouped together by an <code>'[=altr=]'</code> (see [[#altr-group]])

This way we link to the base spec and add a link to the relevant section in AVIF.

Sorry, I missed your replies. This looks good, thank you for the changes.

index.bs

leo-barnes · 2024-10-09T10:09:37Z

index.bs

@@ -1228,7 +1234,7 @@ Consider the following:

 This is equivalent to the following postfix notation (parentheses for clarity):

-<math><msub><mi>sample</mi><mi>output</mi></msub><mo>=</mo><mo>(</mo><mn>256</mn><mspace width="1ch"/><msub><mi>sample</mi><mn>1</mn></msub><mo>×</mo><mo>)</mo><msub><mi>sample</mi><mn>2</mn></msub><mo>+</mo></math>
+<math><msub><mi>sample</mi><mi>output</mi></msub><mo>=</mo><mo>(</mo><mn>256</mn><mspace width="0.5em"/><msub><mi>sample</mi><mn>1</mn></msub><mo>×</mo><mo>)</mo><mspace width="0.5em"/><msub><mi>sample</mi><mn>2</mn></msub><mo>+</mo></math>


"1ch" does not seem to work in Safari. The space simply disappears. "0.5em" seems to work in both Chrome and Safari and gives roughly the same space.

Strange, it should be supported: https://caniuse.com/ch-unit

Yeah, it's weird. Maybe it doesn't work correctly in <math> tags but works in other CSS stuff?

Is it https://caniuse.com/mdn-api_mathmlelement_style?

Good question. That looks like it should be supported in Safari though. I pinged the WebKit folks to see if they know about it.

Turns out it's an issue in Firefox as well. WebKit folks opened an issue for it:
https://bugs.webkit.org/show_bug.cgi?id=281232

index.bs

y-guyon · 2024-10-09T10:51:48Z

index.bs

@@ -1228,7 +1234,7 @@ Consider the following:

 This is equivalent to the following postfix notation (parentheses for clarity):

-<math><msub><mi>sample</mi><mi>output</mi></msub><mo>=</mo><mo>(</mo><mn>256</mn><mspace width="1ch"/><msub><mi>sample</mi><mn>1</mn></msub><mo>×</mo><mo>)</mo><msub><mi>sample</mi><mn>2</mn></msub><mo>+</mo></math>
+<math><msub><mi>sample</mi><mi>output</mi></msub><mo>=</mo><mo>(</mo><mn>256</mn><mspace width="0.5em"/><msub><mi>sample</mi><mn>1</mn></msub><mo>×</mo><mo>)</mo><mspace width="0.5em"/><msub><mi>sample</mi><mn>2</mn></msub><mo>+</mo></math>


Strange, it should be supported: https://caniuse.com/ch-unit

index.bs

y-guyon · 2024-10-09T10:54:46Z

index.bs

-    - <code>'[=colr=]'</code>
-    - <code>'[=pixi=]'</code>
+In addition to the Image Properties defined in this document, [=AV1 image items=] may also be associated with item properties defined in other specifications such as [[!HEIF]] and [[!MIAF]]. Examples of commonly used item properties are:
+    - <code>'[=colr=]'</code> (<dfn noexport for="AVIF">ColourInformationBox</dfn>)


Why not just refer to ISOBMFF or HEIF directly?

index.bs

Moved box type definition anchors to the root spec listed in table of boxes Added links to all box types in table of boxes Sorted all anchors, removed duplicates Removed duplicated lists of boxes

SHA: 727501a Reason: push, by leo-barnes Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

SHA: 727501a Reason: push, by y-guyon Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

leo-barnes commented Oct 9, 2024

View reviewed changes

leo-barnes requested review from wantehchang, y-guyon, cconcolato and podborski October 9, 2024 10:10

leo-barnes commented Oct 9, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

y-guyon approved these changes Oct 9, 2024

View reviewed changes

y-guyon reviewed Oct 9, 2024

View reviewed changes

index.bs Show resolved Hide resolved

y-guyon approved these changes Oct 15, 2024

View reviewed changes

Fix final review comments

51a1b82

Moved box type definition anchors to the root spec listed in table of boxes Added links to all box types in table of boxes Sorted all anchors, removed duplicates Removed duplicated lists of boxes

leo-barnes force-pushed the u/lbarnes/final_review_corrections branch from 09237f6 to 51a1b82 Compare October 16, 2024 06:52

leo-barnes merged commit 727501a into master Oct 16, 2024

github-actions bot added a commit that referenced this pull request Oct 16, 2024

Fix final review comments (#256)

b6783ba

SHA: 727501a Reason: push, by leo-barnes Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

leo-barnes deleted the u/lbarnes/final_review_corrections branch October 22, 2024 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix final review comments from Leo #256

Fix final review comments from Leo #256

leo-barnes commented Oct 9, 2024 •

edited by pr-preview bot

Loading

leo-barnes Oct 9, 2024

leo-barnes Oct 9, 2024

y-guyon Oct 9, 2024

leo-barnes Oct 9, 2024

y-guyon Oct 9, 2024

leo-barnes Oct 9, 2024

leo-barnes Oct 10, 2024

y-guyon Oct 10, 2024

leo-barnes Oct 10, 2024

leo-barnes Oct 10, 2024

y-guyon Oct 15, 2024

leo-barnes Oct 9, 2024

y-guyon Oct 9, 2024

leo-barnes Oct 9, 2024 •

edited

Loading

y-guyon Oct 10, 2024

leo-barnes Oct 10, 2024

leo-barnes Oct 10, 2024

y-guyon Oct 9, 2024

y-guyon Oct 9, 2024

		@@ -817,12 +823,12 @@ A file containing a <code>'[=pict=]'</code> track compliant with this profile is
		<h2 id="box-lists">Box requirements</h2>

Fix final review comments from Leo #256

Fix final review comments from Leo #256

Conversation

leo-barnes commented Oct 9, 2024 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leo-barnes Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leo-barnes commented Oct 9, 2024 •

edited by pr-preview bot

Loading

leo-barnes Oct 9, 2024 •

edited

Loading