Feedback on the w23988 "Low-overhead image file format" MPEG proposal for HEIF

[wantehchang]

Comments on working draft at https://www.mpeg.org/wp-content/uploads/mpeg_meetings/146_Rennes/w23988.zip

In Section 6.10.2.6, in rows 9 and 10 of the table, the mapping of the orientation value of 6 could use a smaller rotation angle by using the other mirror axis. I.e., map orientation 6 to

• angle 1 (instead of 3) in row 9 and
• axis 1 (instead of 0) in row 10.

---

In Section 6.10.1.2, it woud be good if the variables is_horizontally_centered and is_vertically_centered could have chroma in their names, e.g., chroma_is_horizontally_centered and chroma_is_vertically_centered.

---

Optional: In Section 6.10.1.2, the four variable names dimension_size, codec_config_size_size, item_data_size_size, and metadata_size_size could be renamed dimension_bits, codec_config_size_bits, item_data_size_bits, and metadata_size_bits so that

• the unit (in bits) is self-explanatory, and

• the confusing _size_size is avoided.

In the updated definition of MetaBox, the version 1 part reads:

    } else if (version == 1) {
        // derived specifications may define the syntax within version 1
        bit(8) data[]; // until the end of the box
    }

I am not familiar with the box specification language, but I am wondering if we should leave the body of version == 1 empty. In Section 6.10.1.2, MetaBoxV1(flags) is defined as extending MetaBox(version = 1, flags). Does this mean the fields in the MetaBoxV1(flags) definition are added after the bit(8) data[]; field in MetaBox(version = 1, flags)?

[y-guyon]

In Section 6.10.2.6, in rows 9 and 10 of the table, the mapping of the orientation value of 6 could use a smaller rotation angle by using the other mirror axis.

Fine with me.

In Section 6.10.1.2, it woud be good if the variables is_horizontally_centered and is_vertically_centered could have chroma in their names, e.g., chroma_is_horizontally_centered and chroma_is_vertically_centered.

I agree with this improvement.

Optional: In Section 6.10.1.2, the four variable names dimension_size, codec_config_size_size, item_data_size_size, and metadata_size_size could be renamed dimension_bits, codec_config_size_bits, item_data_size_bits, and metadata_size_bits

I used _size for consistency with other similar fields in HEIF, such as rot_large_field_size for example.
I do not mind using _bits instead, although _size_bits just means "size in bits" and not "bits to store the size" to me.
What about keeping dimension_size but renaming _size_size to _num_bytes_size?

Trailing // bits comments could be used to clarify the unit.

I am not familiar with the box specification language, but I am wondering if we should leave the body of version == 1 empty.

Removing bit(8) data[]; sounds reasonable.
Alternatively the keyword template could be used, but in m68219 it means "extended by derived specifications" rather than "defined by derived specifications".

[wantehchang]

Yannis: My suggestion of renaming dimension_size, codec_config_size_size, item_data_size_size, and metadata_size_size is optional. Since the current names follow the naming convention used for other similar fields in HEIF, let's leave them unchanged.

I am not familiar with the box specification language, so I am afraid that I don't know whether using the keyword template would be better. Do you think the current definition of MetaBoxV1(flags) inherits the bit(8) data[]; field from MetaBox(version = 1, flags)?

[y-guyon]

Yannis: My suggestion of renaming dimension_size, codec_config_size_size, item_data_size_size, and metadata_size_size is optional. Since the current names follow the naming convention used for other similar fields in HEIF, let's leave them unchanged.

I agree with the fact that _size_size is misleading. The first _size refers to the number of bytes of the data chunk and the second _size refers to the number of bits used to signal that byte count.
Another example in ISOBMFF is entry_size, in bytes.

I am not familiar with the box specification language, so I am afraid that I don't know whether using the keyword template would be better. Do you think the current definition of MetaBoxV1(flags) inherits the bit(8) data[]; field from MetaBox(version = 1, flags)?

I think it does.

[wantehchang]

Yes, both _size_size and _size_bits are misleading. I don't really have a good suggestion. Perhaps _size_field_size, which is inspired by this example in ISO BMFF:

aligned(8) class CompactSampleSizeBox
   extends FullBox('stz2', version = 0, 0) {
   unsigned int(24) reserved = 0;
   unsigned int(8) field_size;
   unsigned int(32) sample_count;
   for (i=1; i <= sample_count; i++) {
   unsigned int(field_size) entry_size;
   }
}

or _size_field_width?

Note: The term "width" is used to refer to the number of bits in an integer type in C, e.g., https://en.cppreference.com/w/c/types/integer

[y-guyon]

I am not familiar with the box specification language, so I am afraid that I don't know whether using the keyword template would be better. Do you think the current definition of MetaBoxV1(flags) inherits the bit(8) data[]; field from MetaBox(version = 1, flags)?

m69008 is an another box example that confirms that template is irrelevant here and that data should not be present.