Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1779513002:
media: Add tests for "mp3" codec string (Closed)
|
Created:
4 years, 9 months ago by ddorwin Modified:
4 years, 9 months ago Reviewers:
chcunningham CC:
chromium-reviews, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Add tests for MP3 codec strings
"mp3" is only considered valid for "audio/mpeg". However, due
to bug 592889, all other MPEG containers have the wrong result.
Similarly, "mp4a.6x" variants should not be supported with
"audio/mpeg" but currently produces the wrong result.
Also made some test ordering more consistent/logical and fixed missing spaces.
BUG=592889
Committed: https://crrev.com/1e03f7aebaf957701f8b480c9bb5dc757211047e
Cr-Commit-Position: refs/heads/master@{#380029}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add tests for the RFC-compliant MP3 strings as well as MPEG-2 AAC. #
Total comments: 20
Patch Set 3 : feedback #
Total comments: 1
Messages
Total messages: 14 (6 generated)
Description was changed from ========== media: Add tests for "mp3" codec string "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. BUG=592889 ========== to ========== media: Add tests for "mp3" codec string "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. Also made some test ordering more consistent/logical and fixed missing spaces. BUG=592889 ==========
ddorwin@chromium.org changed reviewers: + chcunningham@chromium.org
https://codereview.chromium.org/1779513002/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1779513002/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:191: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"1\"'")); Moved these 4 lines up. https://codereview.chromium.org/1779513002/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:321: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"1\"'")); Moved these two lines up.
Description was changed from ========== media: Add tests for "mp3" codec string "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. Also made some test ordering more consistent/logical and fixed missing spaces. BUG=592889 ========== to ========== media: Add tests for MP3 codec strings "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. Similarly, "mp4a.6x" variants should not be supported with "audio/mpeg" but currently produces the wrong result. Also made some test ordering more consistent/logical and fixed missing spaces. BUG=592889 ==========
Added tests for the opposite case. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:191: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"1\"'")); Moved these four lines up. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:321: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"1\"'")); Moved these two lines up. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:622: // The next two results are wrong due to https://crbug.com/592889. The other side of the bug.
https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:692: EXPECT_EQ(kMpeg2AacProbably, CanPlay("'video/mp4; codecs=\"mp4a.68\"'")); Does android not support the above 3? https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:753: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"mp3\"'")); This looks like its case 2 in your bug "(2) is not expected. "audio/mp4" and "video/mp4" should only support RFC 6381 compliant strings" Can you add a comment saying this is a bug. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:775: EXPECT_EQ(kMpeg2AacProbably, CanPlay("'video/x-m4v; codecs=\"mp4a.68\"'")); Same question about android support here. I figured this is being decoded in software, so it should be available everywhere prop_codecs=1.... must be missing something. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:830: EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"mp3\"'")); Is this expected, or part of the bug? If part of the bug, needs a comment. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:846: extra line https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:868: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"mp3\"'")); This is case #2 in your bug right? Can you comment that its a bug https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:908: EXPECT_EQ(kPropProbably, CanPlay("'audio/x-m4a; codecs=\"mp3\"'")); This is bug too right? https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:1290: EXPECT_EQ(kHlsProbably, I would line break the probablys from the nots. The comment only applies to the first 3, right? https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:1387: EXPECT_EQ(kHlsProbably, ditto about line break.
https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:692: EXPECT_EQ(kMpeg2AacProbably, CanPlay("'video/mp4; codecs=\"mp4a.68\"'")); On 2016/03/08 23:25:30, chcunningham wrote: > Does android not support the above 3? No. See https://crbug.com/461009. I added this to the definition of this constant above. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:753: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"mp3\"'")); On 2016/03/08 23:25:30, chcunningham wrote: > This looks like its case 2 in your bug > > "(2) is not expected. "audio/mp4" and "video/mp4" should only support RFC 6381 > compliant strings" > > Can you add a comment saying this is a bug. Done. I was trying to minimize the number of comment lines. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:775: EXPECT_EQ(kMpeg2AacProbably, CanPlay("'video/x-m4v; codecs=\"mp4a.68\"'")); On 2016/03/08 23:25:30, chcunningham wrote: > Same question about android support here. I figured this is being decoded in > software, so it should be available everywhere prop_codecs=1.... must be missing > something. See above, but good point about software decoding (for the unified pipeline). I meant to discuss this with dalecurtis. We are already using this constant at line 1190, so this is currently correct. (Since the unified pipeline is not the default, canPlayType() probably fails. When the default is switched, we'll probably need to update the expected result.) https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:830: EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"mp3\"'")); On 2016/03/08 23:25:30, chcunningham wrote: > Is this expected, or part of the bug? If part of the bug, needs a comment. Done. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:846: On 2016/03/08 23:25:30, chcunningham wrote: > extra line Done. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:868: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"mp3\"'")); On 2016/03/08 23:25:30, chcunningham wrote: > This is case #2 in your bug right? Can you comment that its a bug Yes, I added a comment to all of them. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:908: EXPECT_EQ(kPropProbably, CanPlay("'audio/x-m4a; codecs=\"mp3\"'")); On 2016/03/08 23:25:30, chcunningham wrote: > This is bug too right? Done. https://codereview.chromium.org/1779513002/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:1290: EXPECT_EQ(kHlsProbably, On 2016/03/08 23:25:30, chcunningham wrote: > I would line break the probablys from the nots. The comment only applies to the > first 3, right? Done. https://codereview.chromium.org/1779513002/diff/40001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1779513002/diff/40001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:1289: // Android, is the only platform that supports these types, and its HLS FYI, this result does not change with Spitzer.
lgtm
The CQ bit was checked by ddorwin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779513002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779513002/40001
Message was sent while issue was closed.
Description was changed from ========== media: Add tests for MP3 codec strings "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. Similarly, "mp4a.6x" variants should not be supported with "audio/mpeg" but currently produces the wrong result. Also made some test ordering more consistent/logical and fixed missing spaces. BUG=592889 ========== to ========== media: Add tests for MP3 codec strings "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. Similarly, "mp4a.6x" variants should not be supported with "audio/mpeg" but currently produces the wrong result. Also made some test ordering more consistent/logical and fixed missing spaces. BUG=592889 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== media: Add tests for MP3 codec strings "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. Similarly, "mp4a.6x" variants should not be supported with "audio/mpeg" but currently produces the wrong result. Also made some test ordering more consistent/logical and fixed missing spaces. BUG=592889 ========== to ========== media: Add tests for MP3 codec strings "mp3" is only considered valid for "audio/mpeg". However, due to bug 592889, all other MPEG containers have the wrong result. Similarly, "mp4a.6x" variants should not be supported with "audio/mpeg" but currently produces the wrong result. Also made some test ordering more consistent/logical and fixed missing spaces. BUG=592889 Committed: https://crrev.com/1e03f7aebaf957701f8b480c9bb5dc757211047e Cr-Commit-Position: refs/heads/master@{#380029} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1e03f7aebaf957701f8b480c9bb5dc757211047e Cr-Commit-Position: refs/heads/master@{#380029} |
