Add APP2.HT.REV and APP2.HT.IRV constraint sets validation #368

palemieux · 2024-06-21T05:36:44Z

Closes #370

Validate the CPL essence descriptors against the APP2.HT.REV and APP2.HT.IRV constraint sets specified in ST 2067-21, Annex I.

The PR imposes constraints on the B parameter as suggested at SMPTE/st2067-21#7

To be finalized following the June 26, 2024 IMF plugfest.

Started work on Unit Test

davidt-netflix · 2024-06-25T20:57:37Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+                isValid = false;
+            }
+            if (p.csiz[i].xrsiz != 1) {
+                if (i == 2 && p.csiz[i].xrsiz != 2) {


I think this may be a possible off-by-one bug?

If I am correctly interpreting the 2067-21 spec, I believe when the HT-J2K constraints (Annex I) refer to a specific component, these component numbers are one-based, rather than zero-based index values used by Java.

So in the case of the sub-sampling constraint, (Xrsizi=2 for i = {2,3} and XRsizi=1 for other i), I would interpret that as describing the "second and third component". In the java code, that means you would be looking at csiz index where i is 1 or 2, instead of when i is 2 or 3

these component numbers are one-based, rather than zero-based index values used by Java.

Will fix.

Addressed at c057b3e

davidt-netflix · 2024-06-25T21:44:47Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+                        String.format("APP2.HT: invalid vertical sub-sampling for component %d", i));
+                isValid = false;
+            }
+            if (p.csiz[i].xrsiz != 1) {


There are a few combinations of sub-sampling that I don't think the current validation will catch. Some of these may not be realistic examples.

A non-one value for xrsiz for any component other than component2 or component3.
Example: (4, 1, 1, 4)

Case where component3 is 1, but not equal to component2. I believe the spec requires that component2 and component3 must both be 2, if either of them are.
Example: (1, 2, 1).

If component2 and component3 are both the same invalid value, such as 4, then an error will only be reported for component2, but not component3. As a practical matter, this isn't much of a problem, because it will still set isValid = false. But the lack of error reported for component3 might be slightly misleading.
Example: (1, 4, 4)

Addressed at c057b3e

davidt-netflix · 2024-06-26T14:39:20Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+        }
+        /* CAP constraints */
+
+        if (p.cap == null || p.cap.pcap != 131072 || p.cap.ccap == null || p.cap.ccap.length != 1) {


Can you clarify where the pcap value of 131072 comes from?
Is that defined in SMPTE ST 2067-21? Or somewhere else? Thanks for clarifying

ST 2067-21 specifies that Pcapi is 1 for i = 15, and 0 otherwise, which means that pcap = 2^(32-15) per T.800:

davidt-netflix · 2024-06-26T14:55:52Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+        }
+
+        /* progression order */
+        if (p.cod.progressionOrder != 0b00000010)


Is RPCL a requirement? ST 2067-21, Annex I lists Progression order: LRCP, RLCP, RPCL, PCRL, CPRL.
Are those other options allowable also?

Can you provide more context as to why a WARNING is needed here?

RPCL is not a requirement, but a strong recommendation, as indicated in ST 2067-21, Annex I, Note 3.

P.S.: I see no reason to do not anything but RPCL for IMF App 2E.

Ah, that makes sense. That also explains why it is a WARNING.
If any other HT-J2K users disagree with flagging this, we can always revisit this later. But I agree, based on Note 3, this seems like a reasonable recommendation to include by default.

It might be a good idea to introduce an Enum for these binary-encoded fields, just for better readability.

davidt-netflix · 2024-06-26T16:08:38Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+            maxB += 1;
+        }
+
+        int codestreamB = (p.cap.ccap[0] & 0b11111) + 8;


It is probably a good idea to add a comment here, that the relationship of "MAGBp" to "Parameter B" is not always +8.
At least not in the HT-J2K spec. According to ITU-T T.814 , the table in 8.7.3 shows that the +8 rule only works up until MAGB19. After that, the values are less consistent. Referencing the table is required.

However, this is not currently an issue that Photon needs to be concerned with. In ST 2067-21, the largest Parameter B value used, is 21. So for any IMF use-cases, the MAGBp + 8 calculation will work still. But this assumption may eventually fail, if a future IMF update adds support for larger Parameter B values.

The code doesn't necessarily need to change. But it is probably best to document that the codestreamB calculation is actually based on a table in the HT-J2K spec.

davidt-netflix · 2024-06-26T16:16:11Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+        /* magbp */
+
+
+        int maxB = p.csiz[0].ssiz + 2;


I'd suggest adding a comment, explaining that the maxB value, is based on values in Table I.2 and Table I.3.

The maxB calculations are a little hard to follow, since ST 2067-21 doesn't present these as a "formula", but rather just a table of hardcoded values.
What you are doing makes sense after reading through the code several times. It looks like you've got all the values correct. But adding a comment explaining the calculations might make this code easier for others to understand.

Will do.

The code needs additional comments, which I plan to add once we are somewhat happy with the results and the overall flow/structure.

src/main/java/com/netflix/imflibrary/utils/DOMNodeObjectModel.java

davidt-netflix · 2024-06-26T16:37:04Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

Should the errors reported under validateHT use the ErrorLevel NON_FATAL instead of FATAL?
I noticed that the existing validateImageCharacteristics method uses NON_FATAL, in case we wanted to consider matching that, for consistency.

But I don't know enough about HT-J2K myself, to make informed decisions about the severity of each constraint violation. So whatever ErrorLevel you feel is best, go ahead and use that.

I was wondering about this. Couple of questions/data points:

what is the difference between NON_FATAL and FATAL? Does it refer to the ability of the code to continue or to the validity of the codestream? A codestream that deviates from the APP2.HT.REV and APP2.HT.IRV constraints is fatally invalid, but does not stop validation.

is there the notion of WARNING in Photon, i.e., a deviation from a SHOULD/recommendation in the spec?

I will see if I can track down any sort of "official" explanation of the different error levels. They were defined that way long before I started working with Photon, so I don't have firsthand knowledge of the original decision.

As a practical matter, I generally try to match whatever value is already being used in similar code. 😁 But that isn't really a good answer.

Within Photon itself, the biggest difference I can see between NON_FATAL and FATAL, is the error logger hasFatalErrors method. And hasFatalErrors is used in a several places to bail out of validation early, by throwing an exception.

Based on my own observations, it seems that generally a FATAL error is reported when something is not able to be read, or parsing fails due to a structural issue. These are typically cases where the validation is not going to be able to proceed. Examples: Failing to deserialize the PKL file, or Invalid MXF Header Partition.

NON_FATAL errors seem to generally be reported for "Hey, the spec says you can't use that value." problems, which do not necessarily prevent you from running the rest of your validations still. Example: An unsupported width/height reported.

WARNING seems to be considerably less common. It looks like they are mostly used to report values that are not recognized, but are not necessarily prohibited either. I think your use of WARNING for the RCPL recommendation is an excellent example. We should encourage more of these throughout the code.

Like I said, I'll try to track down a more definitive answer. But once we settle on something (and I welcome your feedback, if you have specific preferences), I'll plan to go ahead and document this somewhere "officially", within the Photon code base. That way we can try to keep our usage consistent.

davidt-netflix · 2024-06-26T16:56:25Z

src/main/java/com/netflix/imflibrary/st2067_2/CompositionImageEssenceDescriptorModel.java

@@ -388,6 +400,217 @@ public TransferCharacteristic getTransferCharacteristic() {
        return paletteLayout;
    }

+    static public class J2KHeaderParameters {


This isn't a major problem, but how practical would it be to reuse and enhance the existing JPEG2000PictureSubDescriptor class, rather than defining a new J2KHeaderParameters? My understanding, is that both classes essentially represent the same information, right?

We have similar cases all throughout Photon. Usually, one class will be defined once for a piece of data extracted from the MXF file, and then a second class will be defined elsewhere, to represent the same piece of data, when it is extracted from the CPL. Ideally, both cases should be sharing the same Java class.

This is an area of tech debt that we would like to clean up in a future revision of Photon.
Obviously, all of those other duplicate classes are unrelated to your pull request. And you don't need to change anything right now, especially if that is not a practical change to make.
But it might be worth considering. Changing it now is much easier than reorganizing things in a future refactoring, once people begin to reference J2KHeaderParameters.

This isn't a major problem, but how practical would it be to reuse and enhance the existing JPEG2000PictureSubDescriptor class, rather than defining a new J2KHeaderParameters?

Ok. Will give a shot.

davidt-netflix · 2024-06-26T17:01:25Z

Looks good to me. 👍
I left several comments about minor changes, or code style. But I don't see any major issues. You did a really good job of adding validations for pretty much every constraint in Annex I.

I'll let you consider some of the comments, and give everyone else a chance to review the PR as well. But otherwise I'll plan to mark it approved, and merge in the next few days, whenever you are ready.

danielhdz13-netflix · 2024-07-25T18:41:23Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+        if (codestreamB > maxB) {
+            logger.addError(
+                IMFErrorLogger.IMFErrors.ErrorCodes.APPLICATION_COMPOSITION_ERROR,
+                IMFErrorLogger.IMFErrors.ErrorLevels.FATAL,


Taking this issue into consideration, should we lower the fatality to a WARNING here?

Yes.

A B value over, say, 24 should probably be an error since it should never happen and could break software decoders.

fschleich · 2024-08-05T16:54:22Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+
+        return isValid;
+    }
+


Suggested change

/**

* @deprecated Instead use {@link #isValidJ2KProfile(CompositionImageEssenceDescriptorModel imageDescriptor, IMFErrorLogger logger)}

*/

@Deprecated

public static boolean isValidJ2KProfile(CompositionImageEssenceDescriptorModel imageDescriptor) {

return isValidJ2KProfile(imageDescriptor, new com.netflix.imflibrary.IMFErrorLoggerImpl());

}

To avoid breaking changes to public interface for now.

danielhdz13-netflix · 2024-08-05T20:27:34Z

src/main/java/com/netflix/imflibrary/st2067_2/Application2E2021.java

+        if (codestreamB > maxB) {
+            logger.addError(
+                IMFErrorLogger.IMFErrors.ErrorCodes.APPLICATION_COMPOSITION_ERROR,
+                IMFErrorLogger.IMFErrors.ErrorLevels.FATAL,


Suggested change

IMFErrorLogger.IMFErrors.ErrorLevels.FATAL,

codestreamB > 24 ? IMFErrorLogger.IMFErrors.ErrorLevels.FATAL : IMFErrorLogger.IMFErrors.ErrorLevels.WARNING,

fschleich · 2024-08-20T22:30:17Z

PR replaced by PR #375 (merged)

palemieux added 15 commits June 20, 2024 16:18

First pass

4cc6ed0

Add CAP parsing

d19cd53

Add COD

eac5f5d

Fix validation

322dcb1

Added QCD

b76ba8e

Started work on Unit Test

Expand Unit Tests

f9ce475

Add more tests

2ff4404

Exclude unread fields warning from non-test CI

428f20f

Add validation tests

96287e8

Added tests

0a5c44b

MAGB validation

8abbd3a

Improve HT error reporting

a03f107

Fix bugs

bd6980a

Fix HTREV bug

e5feb62

Clean-up MAGB check

1fdf591

danielhdz13-netflix marked this pull request as ready for review June 25, 2024 20:54

danielhdz13-netflix requested review from davidt-netflix and danielhdz13-netflix June 25, 2024 20:55

danielhdz13-netflix marked this pull request as draft June 25, 2024 20:56

davidt-netflix reviewed Jun 25, 2024

View reviewed changes

Addresses Netflix#368 (review)

c057b3e

davidt-netflix reviewed Jun 26, 2024

View reviewed changes

src/main/java/com/netflix/imflibrary/utils/DOMNodeObjectModel.java Show resolved Hide resolved

davidt-netflix reviewed Jun 26, 2024

View reviewed changes

danielhdz13-netflix reviewed Jul 25, 2024

View reviewed changes

fschleich reviewed Aug 5, 2024

View reviewed changes

danielhdz13-netflix reviewed Aug 5, 2024

View reviewed changes

palemieux marked this pull request as ready for review August 14, 2024 15:48

fschleich mentioned this pull request Aug 15, 2024

Add App2E HTJ2K constraints validation #375

Merged

fschleich closed this Aug 20, 2024

fschleich mentioned this pull request Aug 28, 2024

Add support for MGA S-ADM Virtual Tracks (SMPTE ST 2067-203) #376

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add APP2.HT.REV and APP2.HT.IRV constraint sets validation #368

Add APP2.HT.REV and APP2.HT.IRV constraint sets validation #368

palemieux commented Jun 21, 2024 •

edited

Loading

davidt-netflix Jun 25, 2024

davidt-netflix Jun 25, 2024

palemieux Jun 25, 2024

palemieux Jun 26, 2024

davidt-netflix Jun 25, 2024

palemieux Jun 26, 2024

davidt-netflix Jun 26, 2024

palemieux Jun 26, 2024

davidt-netflix Jun 26, 2024

danielhdz13-netflix Jun 26, 2024

palemieux Jun 26, 2024 •

edited

Loading

davidt-netflix Jun 26, 2024

danielhdz13-netflix Jun 26, 2024

davidt-netflix Jun 26, 2024 •

edited

Loading

palemieux Jun 26, 2024

davidt-netflix Jun 26, 2024

palemieux Jun 26, 2024

davidt-netflix Jun 26, 2024

palemieux Jun 26, 2024

davidt-netflix Jun 26, 2024

davidt-netflix Jun 26, 2024

palemieux Jun 26, 2024

davidt-netflix commented Jun 26, 2024

danielhdz13-netflix Jul 25, 2024

palemieux Jul 25, 2024

fschleich Aug 5, 2024

fschleich Aug 5, 2024

danielhdz13-netflix Aug 5, 2024

fschleich commented Aug 20, 2024

+   /**
+     * @deprecated Instead use {@link #isValidJ2KProfile(CompositionImageEssenceDescriptorModel imageDescriptor, IMFErrorLogger logger)}
+     */
+    @Deprecated
+    public static boolean isValidJ2KProfile(CompositionImageEssenceDescriptorModel imageDescriptor) {
+        return isValidJ2KProfile(imageDescriptor, new com.netflix.imflibrary.IMFErrorLoggerImpl());
+    }

	IMFErrorLogger.IMFErrors.ErrorLevels.FATAL,
	codestreamB > 24 ? IMFErrorLogger.IMFErrors.ErrorLevels.FATAL : IMFErrorLogger.IMFErrors.ErrorLevels.WARNING,

Add APP2.HT.REV and APP2.HT.IRV constraint sets validation #368

Add APP2.HT.REV and APP2.HT.IRV constraint sets validation #368

Conversation

palemieux commented Jun 21, 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

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

palemieux Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidt-netflix Jun 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidt-netflix commented Jun 26, 2024

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

fschleich commented Aug 20, 2024

palemieux commented Jun 21, 2024 •

edited

Loading

palemieux Jun 26, 2024 •

edited

Loading

davidt-netflix Jun 26, 2024 •

edited

Loading