Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1292703005:
Remove constraint_set_flag checks from ParseH264CodecID(). (Closed)
|
Created:
5 years, 4 months ago by sandersd (OOO until July 31) Modified:
5 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove constraint_set_flag checks from ParseH264CodecID().
The spec does not require these flags to be set; they exist to signal compatability with a profile without explicitly being that profile.
BUG=522744
Committed: https://crrev.com/96f574ad73be4ec5b7b8e80f9d16802dc3a537c7
Cr-Commit-Position: refs/heads/master@{#344666}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 24 (10 generated)
sandersd@chromium.org changed reviewers: + ddorwin@chromium.org
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292703005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292703005/1
ddorwin@chromium.org changed reviewers: + wolenetz@chromium.org
LGTM, but we should see if wolenetz@ has anything to say. https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:610: // The first two digits must be 4D. The second two must be valid hex, but The third digit must be valid hex. The fourth must be 0. https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:614: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.4D001E\"'")); If 4D40 is the "canonical" version (?), we should probably continue to test that as well. https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:623: // The first two digits must be 64. The second two must be valid hex, but ditto https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:679: // The first two digits must be 4D. The second two must be valid hex, but same as above
https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:610: // The first two digits must be 4D. The second two must be valid hex, but On 2015/08/20 20:18:05, ddorwin wrote: > The third digit must be valid hex. The fourth must be 0. Done. https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:614: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.4D001E\"'")); On 2015/08/20 20:18:05, ddorwin wrote: > If 4D40 is the "canonical" version (?), we should probably continue to test that > as well. Not in any real sense. It's a popular way to mark videos, but the particular digit has no meaning in this case. It is significant when the profile (first two digits) are something else, but that meaning has evolved as the spec has been updated and we don't currently handle those cases. We would want tests like that if we do add such support. (Note that such cases only increase the list of supported values, never decrease it.) https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:623: // The first two digits must be 64. The second two must be valid hex, but On 2015/08/20 20:18:05, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:679: // The first two digits must be 4D. The second two must be valid hex, but On 2015/08/20 20:18:05, ddorwin wrote: > same as above Done.
lgtm https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1292703005/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:614: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.4D001E\"'")); On 2015/08/20 20:26:06, sandersd wrote: > On 2015/08/20 20:18:05, ddorwin wrote: > > If 4D40 is the "canonical" version (?), we should probably continue to test > that > > as well. > > Not in any real sense. It's a popular way to mark videos, but the particular > digit has no meaning in this case. > > It is significant when the profile (first two digits) are something else, but > that meaning has evolved as the spec has been updated and we don't currently > handle those cases. We would want tests like that if we do add such support. > (Note that such cases only increase the list of supported values, never decrease > it.) If it's common, it's probably good to include here. It's unlikely, but if someone added a check to exclude duplicate constraint bits or something like that, such a test would catch that.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/1292703005/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292703005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292703005/40001
The CQ bit was unchecked by sandersd@chromium.org
lgtm % 2 nits: https://codereview.chromium.org/1292703005/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1292703005/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:597: // The first two digits must be 42. The second (constraint_set_flags) must Here and below, s/second/third digit, and s/fourth/fourth digit/ ? https://codereview.chromium.org/1292703005/diff/20001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1292703005/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:500: // avc1.64x0yy - H.264 High The comment in previous l.494 still seems relevant. Or am I misreading that in the spec (or is my spec outdated), too?
https://codereview.chromium.org/1292703005/diff/20001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1292703005/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:500: // avc1.64x0yy - H.264 High On 2015/08/20 21:06:43, wolenetz wrote: > The comment in previous l.494 still seems relevant. Or am I misreading that in > the spec (or is my spec outdated), too? Never mind (per our chat, l502 in this patch set retains enough of this context from previous l.494 comment, though we consider such as "ambiguous" signalling).
https://codereview.chromium.org/1292703005/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1292703005/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:597: // The first two digits must be 42. The second (constraint_set_flags) must On 2015/08/20 21:06:43, wolenetz wrote: > Here and below, s/second/third digit, and s/fourth/fourth digit/ ? Done. https://codereview.chromium.org/1292703005/diff/20001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1292703005/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:500: // avc1.64x0yy - H.264 High On 2015/08/20 21:06:43, wolenetz wrote: > The comment in previous l.494 still seems relevant. Or am I misreading that in > the spec (or is my spec outdated), too? That's correct, but I believe the existing comment below covers the important parts of that.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1292703005/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292703005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292703005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292703005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292703005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/96f574ad73be4ec5b7b8e80f9d16802dc3a537c7 Cr-Commit-Position: refs/heads/master@{#344666} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
