Add support for nclc colour type in colr box

[libark]

No description provided.

[coderabbitai]

Walkthrough

A new variant Nclc has been added to the Colr enum in the colr.rs file, containing three u16 fields: colour_primaries, transfer_characteristics, and matrix_coefficients. The decode_body function now employs a match statement to handle multiple colour box types—NCLX, NCLC, PROF, and RICC—with corresponding parsing logic for each. Constants were introduced for these box types. The encode_body function was updated to serialize the new Nclc variant, while existing NCLX encoding functionality remains unchanged. The changes add 54 lines and remove 21 lines of code.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to evaluate whether the description relates to the changeset.	Add a pull request description that explains the purpose, context, and rationale for adding nclc colour type support to help reviewers understand the change.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Add support for nclc colour type in colr box' directly and clearly describes the main change: adding support for a new nclc colour type variant in the colr box implementation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bradh]

Thanks for the contribution.

Where does nclc as a colr box variation come from? I don't see it in ISO/IEC 14496-12:2022 Section 12.1.5.2.

Can you update the PR with a bit of background?

Is there a convention for the nclc and full-range? Could we map it to the same structure as Nclx ?

[libark]

Thanks for the contribution.

Where does nclc as a colr box variation come from? I don't see it in ISO/IEC 14496-12:2022 Section 12.1.5.2.

Can you update the PR with a bit of background?

Is there a convention for the nclc and full-range? Could we map it to the same structure as Nclx ?

nclc is not defined in ISO BMFF. It originates from Apple QuickTime. This predates the standardization of CICP (ISO/IEC 23001-8). When MP4 later adopted the colr box as an extensible mechanism, nclc was not standardized but became widely supported by convention for compatibility with QuickTime/MOV files. As a result, it is commonly encountered in both MOV and MP4 files despite not being mentioned in ISO/IEC 14496-12.

FFmpeg treats nclc explicitly as a QuickTime-derived extension, see mov_read_colr() in libavformat/mov.c for a reference implementation.

When nclc is used, no color range is signaled. In practice, this should be interpreted as video (limited) range.

[bradh]

Would it make sense to parse it in as basically an abbreviated form of nclx - which is what GStreamer qtdemux does, along with FFmpeg as you have identified - but not to write it (and instead write nclx)?

I would prefer not to encourage use of quicktime in 2025...

[libark]

Would it make sense to parse it in as basically an abbreviated form of nclx - which is what GStreamer qtdemux does, along with FFmpeg as you have identified - but not to write it (and instead write nclx)?

I would prefer not to encourage use of quicktime in 2025...

Support for writing nclc is also required. Whether nclx or nclc is written depends on whether the container format is MP4 or MOV. This is the same behavior as in FFmpeg and GStreamer.

FFmpeg/libavformat/movenc.c

    ffio_wfourcc(pb, "colr");
    if (track->mode == MODE_MP4 || track->mode == MODE_AVIF)
        ffio_wfourcc(pb, "nclx");
    else
        ffio_wfourcc(pb, "nclc");
    avio_wb16(pb, track->par->color_primaries);
    avio_wb16(pb, track->par->color_trc);
    avio_wb16(pb, track->par->color_space);
    if (track->mode == MODE_MP4 || track->mode == MODE_AVIF) {
        int full_range = track->par->color_range == AVCOL_RANGE_JPEG;
        avio_w8(pb, full_range << 7);
    }

gstreamer/subprojects/gst-plugins-good/gst/isomp4/atoms.c

if (is_mp4)
    GST_WRITE_UINT32_LE (data, FOURCC_nclx);
  else
    GST_WRITE_UINT32_LE (data, FOURCC_nclc);

  GST_WRITE_UINT16_BE (data + 4, primaries);
  GST_WRITE_UINT16_BE (data + 6, transfer_function);
  GST_WRITE_UINT16_BE (data + 8, matrix);

  if (is_mp4) {
    GST_WRITE_UINT8 (data + 10,
        colorimetry->range == GST_VIDEO_COLOR_RANGE_0_255 ? 0x80 : 0x00);
  }

[bradh]

Would it make sense to parse it in as basically an abbreviated form of nclx - which is what GStreamer qtdemux does, along with FFmpeg as you have identified - but not to write it (and instead write nclx)?
I would prefer not to encourage use of quicktime in 2025...

Support for writing nclc is also required. Whether nclx or nclc is written depends on whether the container format is MP4 or MOV. This is the same behavior as in FFmpeg and GStreamer.

Those are really showing the origin of the code, where MOV code was adapted to MP4.

It doesn't occur for more contemporary code, such as https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/blob/main/mux/isobmff/src/isobmff/boxes.rs?ref_type=heads#L1707-1735

I still don't understand the use case for writing nclc (implicitly, the use case for writing MOV) when there are standardised alternatives.

[libark]

Would it make sense to parse it in as basically an abbreviated form of nclx - which is what GStreamer qtdemux does, along with FFmpeg as you have identified - but not to write it (and instead write nclx)?
I would prefer not to encourage use of quicktime in 2025...

Support for writing nclc is also required. Whether nclx or nclc is written depends on whether the container format is MP4 or MOV. This is the same behavior as in FFmpeg and GStreamer.

Those are really showing the origin of the code, where MOV code was adapted to MP4.

It doesn't occur for more contemporary code, such as https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/blob/main/mux/isobmff/src/isobmff/boxes.rs?ref_type=heads#L1707-1735

I still don't understand the use case for writing nclc (implicitly, the use case for writing MOV) when there are standardised alternatives.

MOV is structurally ISOBMFF-like, but it is not ISOBMFF-compliant. This choice depends on whether the project enforces strict ISO BMFF support or also supports MOV for compatibility. From a general usability and compatibility perspective, I think choosing the latter is preferable.

[bradh]

MOV is structurally ISOBMFF-like, but it is not ISOBMFF-compliant. This choice depends on whether the project enforces strict ISO BMFF support or also supports MOV for compatibility. From a general usability and compatibility perspective, I think choosing the latter is preferable.

I agree that ISOBMFF is based on Quicktime. Because there is some bleed-over from legacy implementations into MP4 products, we do support some elements of MOV. So mp4-atom not strictly an ISOBMFF implementation (especially on reading).

Against that, I see MOV as obsolete, and writing it is a not an explicit goal. I'm not against having some support (i.e. additional complexity) if it serves a valid purpose. However I'm not understanding that that purpose.

[libark]

I think the MOV format is not obsolete. On the latest versions of macOS, screen recordings are still generated in the MOV format. If mp4-atom is used to develop an application that generates MOV files, supporting nclc will be necessary.

[bradh]

OK. Lets move forward.

If you add a unit test (ideally based on data extracted from contemporary MOV) demonstrating round-trip, I'll apply this.

Since its non-standard, consider adding an info-level log message about use of quicktime extensions. That is not a requirement (since we didn't do it anywhere else, yet), just a nice to have.

[kixelated]

Since its non-standard, consider adding an info-level log message about use of quicktime extensions. That is not a requirement (since we didn't do it anywhere else, yet), just a nice to have.

Seems like overkill TBH. Logging doesn't change the maintenance cost.

Should colr be #[non_exhaustive] anyway? I think we should do that in general, and it would make supporting QuickTime more palatable because it won't break existing code.