[AV1] Incorrect checks for chroma_sample_position in ParseAv1CodecId |
|||||
Issue descriptionhttps://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
,
Aug 29
Per the av1 spec you link, your concern seems correct. Tom, can you verify?
,
Aug 29
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
,
Aug 29
Seems like we can just delete: "(subsampling_x == '1' && subsampling_y == '1' && chroma_sample_position == '0')" ?
,
Aug 30
Updated labels since we'll want to merge this probably.
,
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
,
Aug 30
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.
,
Aug 30
+MR-70 since this will be needed for AV1 launch.
,
Aug 30
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
,
Aug 30
Ah nevermind, sorry thought branch already happened on Tuesday. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by dalecur...@chromium.org
, Aug 29Status: Assigned (was: Untriaged)