New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 878922 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

[AV1] Incorrect checks for chroma_sample_position in ParseAv1CodecId

Project Member Reported by kqyang@chromium.org, Aug 29

Issue description

https://cs.chromium.org/chromium/src/media/base/video_codecs.cc?rcl=137dc8bc3bed0cbdde708372c4f97aa3732946db&l=399

  if (((subsampling_x == '0' || subsampling_y == '0') &&
       chroma_sample_position != '0') ||
      (subsampling_x == '1' && subsampling_y == '1' &&
       chroma_sample_position == '0')) {
    DVLOG(3) << __func__ << " Invalid chroma subsampling (" << fields[5] << ")";
    return false;
  }

When subsampling_x and subsampling_y are 1, the code does not allow chroma_sample_position to be 0.

In fact, it is possible that chroma_sample_position could be 0. I think we should allow chroma_sample_position to be 0 when subsampling_x == '1' && subsampling_y == '1'.

(1) When mono_chrome == 1, subsampling_x = 1, subsampling_y = 1, chroma_sample_position = 0 [1]

if ( mono_chrome ) {
  color_range f(1)
  subsampling_x = 1
  subsampling_y = 1
  chroma_sample_position = CSP_UNKNOWN
  separate_uv_delta_q = 0
  return
}

(2) Even when mono_chrome is not 1, it seems possible that chroma_sample_position could be 0 [2]

chroma_sample_position of 0 means: Unknown (in this case the source video transfer function must be
signaled outside the AV1 bitstream).

I am not familiar with AV1 color configuration so I could be wrong.

(3) AV1 Codec ISO Media File Format Binding [3] specifies the values shall come from Sequence Header OBU in the bit stream. The binding document [4] also says that the fields in AV1CodecConfigurationRecord shall come from Sequence Header OBU in the bit stream. In file https://cs.chromium.org/chromium/src/media/test/data/bear-av1.mp4?q=bear-av1.mp4&sq=package:chromium&dr, AV1CodecConfigurationRecord has subsampling_x = 1, subsampling_y = 1 and chroma_sample_position = 0. It is actually a bug in the content as in the bit stream, chroma_sample_position is 2. However, it could be a problem in a lot of contents; if content packagers use AV1CodecConfigurationRecord to derive the codec string instead of parsing the actual bit stream, the result content would not play in Chrome.

(4) chromaSubsampling field (== subsampling_x + subsampling_y + chroma_sample_position) is optional in the codecs parameter string, thus Chrome allows it to be absent, which implies unknown subsampling_x, unknown subsampling_y and unknown chroma_sample_position. It is ironic that Chrome does not allow chroma_sample_position to be unknown while subsampling_x and subsampling_y are known.


[1] https://aomediacodec.github.io/av1-spec/av1-spec.pdf#page=44 5.5.2 Color config syntax
[2] https://aomediacodec.github.io/av1-spec/av1-spec.pdf#page=131 6.4.2. Color config semantics
[3] https://aomediacodec.github.io/av1-isobmff/#codecsparam
[4] https://aomediacodec.github.io/av1-isobmff/#av1codecconfigurationbox-syntax
 
Cc: tomfinegan@chromium.org
Status: Assigned (was: Untriaged)
I'll have to dig more to understand your concern clearly, but this was specifically clarified by the spec author to be the code that I have (assuming I didn't make a mistake):

https://github.com/AOMediaCodec/av1-isobmff/pull/37
Per the av1 spec you link, your concern seems correct. Tom, can you verify?
I think KongQun is right, and TBH I'm not exactly surprised that it turns out there's a problem in bear-av1.mp4 given the hoops I had to jump through to get FFmpeg to produce a file that MSE was happy with. CSP_UNKNOWN is most certainly valid for profile 0 and for monochrome. 

Note that the source of this confusion is likely a problem that's since been fixed in ISOBMFF. When the referenced code was written it was correct[1] according to the example in the format binding doc. 

Anyway, we have two issues here:
1. bear-av1.mp4 needs to be re(re(re))generated.
2. The referenced code needs to be updated to match the latest binding spec.

[1] https://github.com/AOMediaCodec/av1-isobmff/pull/66/files
Seems like we can just delete:

"(subsampling_x == '1' && subsampling_y == '1' && chroma_sample_position == '0')" 

?
Labels: -Pri-2 M-70 Pri-1
Updated labels since we'll want to merge this probably.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a72f22228f14e9b362cbae7113c5190961deda6

commit 5a72f22228f14e9b362cbae7113c5190961deda6
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Aug 30 02:48:52 2018

Relax AV1 codec string checks to conform to current spec.

It's unclear if this was wrong originally or later relaxed, but the
spec certainly says that the chroma_sample_position may be 0 at
any time.

BUG= 878922 
TEST=updated tests.

Change-Id: Ib39424a3e2b620a7650eb5bd9d9b4dee65f8edf6
Reviewed-on: https://chromium-review.googlesource.com/1196164
Reviewed-by: Tom Finegan <tomfinegan@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587427}
[modify] https://crrev.com/5a72f22228f14e9b362cbae7113c5190961deda6/media/base/video_codecs.cc
[modify] https://crrev.com/5a72f22228f14e9b362cbae7113c5190961deda6/media/base/video_codecs_unittest.cc

Thanks for the quick fix. I filed another issue for the problem in the file in https://bugs.chromium.org/p/chromium/issues/detail?id=879263.
Labels: Merge-Request-70
+MR-70 since this will be needed for AV1 launch.
Project Member

Comment 10 by sheriffbot@chromium.org, Aug 30

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: We don't branch M70 until 2018-08-30.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Review -Merge-Review-70
Status: Fixed (was: Assigned)
Ah nevermind, sorry thought branch already happened on Tuesday.

Sign in to add a comment