Spec/Doc/Code mismatch for detecting filtered pictures in profiles/levels

[peterderivaz]

Experiment F408 introduced a test to see whether an intra frame used global intrabc in conjunction with any in-loop filter.

The text from the document was:

GlobalIntraBCFilteredPicture is defined to indicates whether a frame is coded with the global Intra-BC enabled in conjunction with any in-loop filter. It’s value is set equal to 1 if the frame satisfies the condition (allow_global_intrabc == 1) && (loop_filter_level[ 0 ] != 0 || loop_filter_level[ 1 ] != 0 || cdef_frame_enable != 0 || FrameRestorationType[0] != RESTORE_NONE || FrameRestorationType[1] != RESTORE_NONE || FrameRestorationType[2] != RESTORE_NONE || frame_filters_on[ 0 ] != 0 || frame_filters_on[ 1 ] != 0 || frame_filters_on[ 2 ] != 0 || gdf_frame_enable != 0). Otherwise, it is set equal to 0.

Note that this does not detect CCSO filtering.

I think this was a mistake in the document, but it was corrected in the reference code and the specification in two slightly different ways.

In the reference code the condition is in:

bool is_filter_enabled_frame(const AV2_COMMON *const cm) {
  bool inloop_filtering_enabled =
      cm->lf.apply_deblocking_filter[0] != 0 ||
      cm->lf.apply_deblocking_filter[1] != 0 ||
      cm->cdef_info.cdef_frame_enable != 0 ||
      cm->cur_frame->ccso_info.ccso_enable[0] != 0 ||
      cm->cur_frame->ccso_info.ccso_enable[1] != 0 ||
      cm->cur_frame->ccso_info.ccso_enable[2] != 0 ||
      cm->rst_info[0].frame_restoration_type != RESTORE_NONE ||
      cm->rst_info[1].frame_restoration_type != RESTORE_NONE ||
      cm->rst_info[2].frame_restoration_type != RESTORE_NONE ||
      cm->gdf_info.gdf_mode != 0;

  return inloop_filtering_enabled;
}

while the specification had the constraint added in #159 by @LuongQCT as:

• InloopFilteringEnabled for a particular Frame is set equal to 1 if apply_deblocking_filter[ 0 ] != 0 || apply_deblocking_filter[ 1 ] != 0 || cdef_frame_enable != 0 || ccso_frame_flag != 0 || ccso_planes[ 0 ] != 0 || ccso_planes[ 1 ] != 0 || ccso_planes[ 2 ] != 0 || FrameRestorationType[ 0 ] != RESTORE_NONE || FrameRestorationType[ 1 ] != RESTORE_NONE || FrameRestorationType[ 2 ] != RESTORE_NONE || gdf_frame_enable != 0. Otherwise, it is set equal to 0.

In particular, consider the case when ccso_frame_flag is equal to 1, but ccso_planes is equal to 0.

In this case:

1. No CCSO filtering will actually be applied
2. The reference code will set inloop_filtering_enabled to 0
3. The spec will set InloopFilteringEnabled to 1

So the specification and the reference code may disagree on whether the stream is considered legal or not.

This is an unusual case in that setting ccso_frame_flag to 0 is intended to be a faster way of setting all ccso_planes to 0, so it is odd to have ccso_frame_flag set to 1 in combination with all ccso_planes equal to 0.

Options for resolving the mismatch:

A. Change the reference code to match the spec
B. Change the specification to match the reference code
C. Change the specification (and reference code) to say that at least one of entry of ccso_planes must be non-zero when ccso_frame_flag is true

[LuongQCT]

Thanks @peterderivaz for catching that. I have a question: Do we have the case if ccso_frame_flag = true and all entry of ccso_planes = 0? If not, I think the current text and spec work correctly.
To be save and avoid any confusions, I would agree with a fix that we match the words in text and code, and (B) is the cleanest solution.
Thanks

[LuongQCT]

A is also good solution that we don't need to change the spec.

[peterderivaz]

Do we have the case if ccso_frame_flag = true and all entry of ccso_planes = 0?

I am not quite sure what you mean by this.

If you mean "Does the specification allow this case?", the answer is "Yes".

If you mean "Does the reference encoder use this case?", I think the answer is "No" judging from this code in pickccso.c:

cm->ccso_info.ccso_frame_flag = cm->ccso_info.ccso_enable[0];
  if (num_planes > 1) {
    ...
    cm->ccso_info.ccso_frame_flag |= cm->ccso_info.ccso_enable[1];
    cm->ccso_info.ccso_frame_flag |= cm->ccso_info.ccso_enable[2];
  }

If you mean "Does the HDR Nova bitstream generator use this case?", the answer is "Yes" (because this is how I found the mismatch).

[LuongQCT]

Thanks for clarifying. Do you think we should have an input proposal for the CWG and then list the above solutions? The group can then choose one.

[peterderivaz]

Do you think we should have an input proposal for the CWG and then list the above solutions?

Yes please.

With any normative change this late in the process there is always a chance that someone's hardware implementation relies on the previous behaviour so it is good to check with the group first.