Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
626163002:
MSE: Relax H.264 Baseline mimetype profile recognition (Closed)
DescriptionMSE: Relax H.264 Baseline mimetype profile recognition
This change lets "avc[13].42y0xx", where y >= 0x8 and xx is a valid
level, be an unambiguously matched H.264 Baseline profile. This relaxes
the previous requirement of a constrained baseline where y had to be
0xE.
CodecParameter | Before | Now
----------------------------------------------------------------------------------------------------
avc[13].42y0xx | probably if y==E, maybe otherwise | probably if y is in [89ABCDEF], maybe otherwise
BUG=408552
R=rsleevi@chromium.org,qinmin@chromium.org,dalecurtis@chromium.org
TBR=xhwang@chromium.org
TEST=Updated MediaCanPlayTypeTest.CodecSupportTest_*
Committed: https://crrev.com/0a0cad0dfcd0c8ea77352fb23bd2e955744355d5
Cr-Commit-Position: refs/heads/master@{#298163}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address dalecurtis@'s nits (including windows compile error) #Patch Set 3 : address xhwang@'s offline comment #Patch Set 4 : Address remainder of PS1-3 comments, still allow constraint_set3_flag set in baseline #Messages
Total messages: 23 (3 generated)
wolenetz@chromium.org changed reviewers: + dalecurtis@chromium.org
PTAL: rsleevi: net/ OWNER qinmin: all, this CL also relaxes Chrome for Android codec support matching dalecurtis: content/browser/media OWNER I'd like to land this ASAP, so please let me know if you're not available to CR this today. Thanks!
https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:927: // Returns true iff |profile_str| conforms to hex string "42y0": y >= 8. Taken Since this is just 8,9 -- why not explicitly enumerate them instead of using a function?
On 2014/10/03 21:54:07, DaleCurtis wrote: > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc > File net/base/mime_util.cc (right): > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... > net/base/mime_util.cc:927: // Returns true iff |profile_str| conforms to hex > string "42y0": y >= 8. Taken > Since this is just 8,9 -- why not explicitly enumerate them instead of using a > function? This is actually 8,9,A,...,F (hex string).
https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:927: // Returns true iff |profile_str| conforms to hex string "42y0": y >= 8. Taken On 2014/10/03 21:54:07, DaleCurtis wrote: > Since this is just 8,9 -- why not explicitly enumerate them instead of using a > function? (replying again, in context): This is actually 8,9,A,...,F (hex string).
On 2014/10/03 22:27:52, wolenetz wrote: > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc > File net/base/mime_util.cc (right): > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... > net/base/mime_util.cc:927: // Returns true iff |profile_str| conforms to hex > string "42y0": y >= 8. Taken > On 2014/10/03 21:54:07, DaleCurtis wrote: > > Since this is just 8,9 -- why not explicitly enumerate them instead of using a > > function? > > (replying again, in context): > This is actually 8,9,A,...,F (hex string). I'll have a patch uploaded shortly that fixes the windows compile warning.
lgtm % nits https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:927: // Returns true iff |profile_str| conforms to hex string "42y0": y >= 8. Taken On 2014/10/03 21:54:07, DaleCurtis wrote: > Since this is just 8,9 -- why not explicitly enumerate them instead of using a > function? Ahh, sorry the CL description makes it clear, but this is not clear that y can be [8..F]. Can you clarify the comment? https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:938: return (constraint_set_bits & 0x8); Just use >= 8. Let the compiler handle such an optimization :)
Thanks for CR, Dale. rsleevi@ and qinmin@, PTAL @ PS2 https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:927: // Returns true iff |profile_str| conforms to hex string "42y0": y >= 8. Taken On 2014/10/03 22:33:56, DaleCurtis wrote: > On 2014/10/03 21:54:07, DaleCurtis wrote: > > Since this is just 8,9 -- why not explicitly enumerate them instead of using a > > function? > > Ahh, sorry the CL description makes it clear, but this is not clear that y can > be [8..F]. Can you clarify the comment? Done. https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:938: return (constraint_set_bits & 0x8); On 2014/10/03 22:33:56, DaleCurtis wrote: > Just use >= 8. Let the compiler handle such an optimization :) Done.
lgtm
In a few seconds, I'll upload a patch set including a clarifying comment suggested by xhwang@.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
https://codereview.chromium.org/626163002/diff/1/content/browser/media/media_... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/626163002/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:408: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.42C01E\"'")); We should test some tricky invalid cases, e.g. avc1.42E11E avc1.42101E avc1.42601E https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:927: // Returns true iff |profile_str| conforms to hex string "42y0": y >= 8. Taken On 2014/10/03 21:54:07, DaleCurtis wrote: > Since this is just 8,9 -- why not explicitly enumerate them instead of using a > function? It's hex, so it can be 'c', 'e' etc. https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:928: // from Annex A and constraint_set0 in ISO-14496-10. Can you mention 7.3.2.1 and 7.4.2.1 in the spec. These are key (in addition to Annex A) to the understanding of this format. Also, it'd be great if you could summarize the spec about how the format is defined. It's always a pain to read this code without looking at the spec. Also, as discussed offline, the spec is hard to understand. What we "choose" to do is basically check the "42" part and make sure constraint_set0 is set. https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || The substr is a bit overkill; will this work? return profile_str[0] == '4' && profile_str[1] == '2' && profile_str[2] & 0x8 && !(profile_str[2] & 0xF1) && profile_str[3] == '0'; https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:955: // Handle parsing H.264 codec IDs as outlined in RFC 6381 The suffix strings are really defined in ISO-14496-10...
https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || On 2014/10/03 23:08:06, xhwang wrote: > The substr is a bit overkill; will this work? > > return > profile_str[0] == '4' && > profile_str[1] == '2' && > profile_str[2] & 0x8 && > !(profile_str[2] & 0xF1) && > profile_str[3] == '0'; Sorry, you still need to convert string to int, for profile_str[2]... but you shouldn't really need substr...
net/ LGTM On 2014/10/03 23:10:29, xhwang wrote: > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc > File net/base/mime_util.cc (right): > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... > net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || > On 2014/10/03 23:08:06, xhwang wrote: > > The substr is a bit overkill; will this work? > > > > return > > profile_str[0] == '4' && > > profile_str[1] == '2' && > > profile_str[2] & 0x8 && > > !(profile_str[2] & 0xF1) && > > profile_str[3] == '0'; > > Sorry, you still need to convert string to int, for profile_str[2]... but you > shouldn't really need substr...
On 2014/10/03 23:17:35, mmenke wrote: > net/ LGTM > > On 2014/10/03 23:10:29, xhwang wrote: > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc > > File net/base/mime_util.cc (right): > > > > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... > > net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || > > On 2014/10/03 23:08:06, xhwang wrote: > > > The substr is a bit overkill; will this work? > > > > > > return > > > profile_str[0] == '4' && > > > profile_str[1] == '2' && > > > profile_str[2] & 0x8 && > > > !(profile_str[2] & 0xF1) && > > > profile_str[3] == '0'; > > > > Sorry, you still need to convert string to int, for profile_str[2]... but you > > shouldn't really need substr... Also agree with that comment. You can just create a stringpiece for profile_str[2] - Just use base::StringPiece(profile_str.c_str() + 2, 1), I believe.
https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || On 2014/10/03 23:10:28, xhwang wrote: > On 2014/10/03 23:08:06, xhwang wrote: > > The substr is a bit overkill; will this work? > > > > return > > profile_str[0] == '4' && > > profile_str[1] == '2' && > > profile_str[2] & 0x8 && > > !(profile_str[2] & 0xF1) && > > profile_str[3] == '0'; > > Sorry, you still need to convert string to int, for profile_str[2]... but you > shouldn't really need substr... Masking with 0xF1 instead of 0xFF means that we would reply "maybe", not "probably" for an otherwise H.264 baseline profile that has constraint_set3_flag set. I'm not clear that constraint_set3_flag being set disqualifies a bitstream from claiming baseline profile (assuming it has profile_idc=0x42=66 and constraint_set0_flag set). Am I missing something in why you chose 0xF1 here?
On 2014/10/03 23:51:05, wolenetz wrote: > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc > File net/base/mime_util.cc (right): > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... > net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || > On 2014/10/03 23:10:28, xhwang wrote: > > On 2014/10/03 23:08:06, xhwang wrote: > > > The substr is a bit overkill; will this work? > > > > > > return > > > profile_str[0] == '4' && > > > profile_str[1] == '2' && > > > profile_str[2] & 0x8 && > > > !(profile_str[2] & 0xF1) && > > > profile_str[3] == '0'; > > > > Sorry, you still need to convert string to int, for profile_str[2]... but you > > shouldn't really need substr... > > Masking with 0xF1 instead of 0xFF means that we would reply "maybe", not > "probably" for an otherwise H.264 baseline profile that has constraint_set3_flag > set. I'm not clear that constraint_set3_flag being set disqualifies a bitstream > from claiming baseline profile (assuming it has profile_idc=0x42=66 and > constraint_set0_flag set). Am I missing something in why you chose 0xF1 here? I think s/0xF1/0xF0/ is what was intended (assuming conversion to int).
On 2014/10/04 00:42:36, wolenetz wrote: > On 2014/10/03 23:51:05, wolenetz wrote: > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc > > File net/base/mime_util.cc (right): > > > > > https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... > > net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || > > On 2014/10/03 23:10:28, xhwang wrote: > > > On 2014/10/03 23:08:06, xhwang wrote: > > > > The substr is a bit overkill; will this work? > > > > > > > > return > > > > profile_str[0] == '4' && > > > > profile_str[1] == '2' && > > > > profile_str[2] & 0x8 && > > > > !(profile_str[2] & 0xF1) && > > > > profile_str[3] == '0'; > > > > > > Sorry, you still need to convert string to int, for profile_str[2]... but > you > > > shouldn't really need substr... > > > > Masking with 0xF1 instead of 0xFF means that we would reply "maybe", not > > "probably" for an otherwise H.264 baseline profile that has > constraint_set3_flag > > set. I'm not clear that constraint_set3_flag being set disqualifies a > bitstream > > from claiming baseline profile (assuming it has profile_idc=0x42=66 and > > constraint_set0_flag set). Am I missing something in why you chose 0xF1 here? > > I think s/0xF1/0xF0/ is what was intended (assuming conversion to int). In which case, no mask is necessary so long as the uint32 representation of profile_str[2] is >= 8.
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/626163002/60001
Thanks for reviews. xhwang@, I'm TBR'ing you regarding the constraint_set3_flag disallowance in mask 0xF1 that you proposed but I didn't use here. https://codereview.chromium.org/626163002/diff/1/content/browser/media/media_... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/626163002/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:408: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.42C01E\"'")); On 2014/10/03 23:08:06, xhwang wrote: > We should test some tricky invalid cases, e.g. > > avc1.42E11E > avc1.42101E > avc1.42601E Done. https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:928: // from Annex A and constraint_set0 in ISO-14496-10. On 2014/10/03 23:08:06, xhwang wrote: > Can you mention 7.3.2.1 and 7.4.2.1 in the spec. These are key (in addition to > Annex A) to the understanding of this format. > > Also, it'd be great if you could summarize the spec about how the format is > defined. It's always a pain to read this code without looking at the spec. > > Also, as discussed offline, the spec is hard to understand. What we "choose" to > do is basically check the "42" part and make sure constraint_set0 is set. Done. https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:933: profile_str.substr(3, 1) != "0" || On 2014/10/03 23:51:04, wolenetz wrote: > On 2014/10/03 23:10:28, xhwang wrote: > > On 2014/10/03 23:08:06, xhwang wrote: > > > The substr is a bit overkill; will this work? > > > > > > return > > > profile_str[0] == '4' && > > > profile_str[1] == '2' && > > > profile_str[2] & 0x8 && > > > !(profile_str[2] & 0xF1) && > > > profile_str[3] == '0'; > > > > Sorry, you still need to convert string to int, for profile_str[2]... but you > > shouldn't really need substr... > > Masking with 0xF1 instead of 0xFF means that we would reply "maybe", not > "probably" for an otherwise H.264 baseline profile that has constraint_set3_flag > set. I'm not clear that constraint_set3_flag being set disqualifies a bitstream > from claiming baseline profile (assuming it has profile_idc=0x42=66 and > constraint_set0_flag set). Am I missing something in why you chose 0xF1 here? I'm keeping to just >= 8 (which requires constraint_set0_flag, and ignores the other 3 flags). https://codereview.chromium.org/626163002/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:955: // Handle parsing H.264 codec IDs as outlined in RFC 6381 On 2014/10/03 23:08:06, xhwang wrote: > The suffix strings are really defined in ISO-14496-10... Done.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 012d76f6a6f80499536d3134c2bb326b5b1fa332
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0a0cad0dfcd0c8ea77352fb23bd2e955744355d5 Cr-Commit-Position: refs/heads/master@{#298163} |
