gpac / ComplianceWarden

A pluggable compliance checker (ISOBMFF, HEIF/MIAF/AVIF, AV1 HDR10+)
https://gpac.github.io/ComplianceWarden-wasm/
Other
16 stars 7 forks source link

AV1-ISOBMFF & HDR10+ Rule Implementations #69

Closed DenizUgur closed 8 months ago

DenizUgur commented 8 months ago

This PR adds missing rules to AV1-ISOBMFF (except CMAF) and HDR10+

DenizUgur commented 8 months ago

The last commit has conflicting rules. That might need some clarification. @rbouqueau what do you think?

rbouqueau commented 8 months ago

@DenizUgur They are not conflicting. HDR10+, when used with ISOBMFF, depends on AV1-ISOBMFF. AV1-ISOBMFF doesn't forbid metadata OBUs in sample data (and in this case expects av1M's presence). HDR10+ just forbids that.

So AV1-ISOBMFF's test vectors will test av1M's presence when there are metadata OBUs in sample data (and may be prefixed with "valid"). The same test vectors should fail the HDR10+ conformance (hence prefixed with "invalid").

Is it clearer?

DenizUgur commented 8 months ago

@DenizUgur They are not conflicting. HDR10+, when used with ISOBMFF, depends on AV1-ISOBMFF. AV1-ISOBMFF doesn't forbid metadata OBUs in sample data (and in this case expects av1M's presence). HDR10+ just forbids that.

Okay so, the invalid file in the last commit is correctly analyzed. There are metadata OBUs in sample data and hence av1M was used. HDR10+ doesn't want av1M here, so an error is raised.

So AV1-ISOBMFF's test vectors will test av1M's presence when there are metadata OBUs in sample data (and may be prefixed with "valid"). The same test vectors should fail the HDR10+ conformance (hence prefixed with "invalid").

For the valid isobmff file under HDR10+, the av1M shouldn't be used. This wouldn't print any errors or warnings for av1hdr10plus but would warn about the missing av1M for av1isobmff. If that's the correct behaviour then I just need to remove av1M from av1hdr10plus/valid_isobmff.asm and it should be okay.

Is that correct?

podborski commented 8 months ago

if that sample group is present the report will put a successful_checks entry in the context of av1isobmff. But it will put an errors entry in the context of av1hdr10+.

this sample group can just mark if metadata OBUs are present in samples. IIRC for AV1 HDR10+ it was disallowed because every sample will have the T.35 metadata OBU and also that group can not be used to differentiate between multiple T.35 metadata OBUs as unsigned int (24) metadata_specific_parameters is not big enough to completely distinguish the flavor of the T.35 message. I don't recall why we completely disallowed it. Now thinking about it maybe the rule should have been written as "shall not be used for identification purposes of the T.35 metadata OBU", as it could still be useful to mark metadata OBUs (currently AV1 spec does not define much, but it could change). Perhaps worth a discussion at AOM STF WG.

@cconcolato perhaps it is worth to also add a note in AV1ISOBMFF mentioning the issue with the T.35 identification using metadata_specific_parameters? Or we wait until the ISOBMFF amendment is published? But I feel like having a note before it is published will not harm.

For this PR we also need to align with the new assert ids based on https://github.com/AOMediaCodec/av1-isobmff/pull/171. I see that Cyril also asked the bikeshed folks about the issue of uniqueness https://github.com/speced/bikeshed/issues/2686

rbouqueau commented 8 months ago

@DenizUgur Let me know when you think I can merge.

DenizUgur commented 8 months ago

@rbouqueau If you think test outputs are correct then you can merge it.

rbouqueau commented 8 months ago

LGTM. I'll ping you if there are concerns.