Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807463003:
Simplify checking for MP3 format (Closed)
|
Created:
3 years, 8 months ago by jrummell Modified:
3 years, 8 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify checking for MP3 format
Assume that anything that starts with "ID3" is a MP3 format and don't
bother validating the content looking for audio frames.
BUG=709163
TEST=added files to MP3 test case
Review-Url: https://codereview.chromium.org/2807463003
Cr-Commit-Position: refs/heads/master@{#463894}
Committed: https://chromium.googlesource.com/chromium/src/+/06147322f84cd6dc908c41224b8e0c93f1da8518
Patch Set 1 #Patch Set 2 : Simplify #
Total comments: 3
Messages
Total messages: 17 (5 generated)
jrummell@chromium.org changed reviewers: + dalecurtis@chromium.org
PTAL. A bit more complicated than the last one.
Can we reuse media/formats/mpeg for this or extract some common functionality? Also is it sufficient to just mark ID3 presence as MP3? Are there any other formats which use ID3 that we'd care about?
On 2017/04/10 19:42:23, DaleCurtis wrote: > Can we reuse media/formats/mpeg for this or extract some common functionality? > Also is it sufficient to just mark ID3 presence as MP3? Are there any other > formats which use ID3 that we'd care about? I thought about using media/formats/mpeg but there's no dependency allowed from media/base (it's the other way). Plus the mpeg code is only included if proprietary_codecs is set. I looked the code to see what it does, but it appears to simply skip over the header, and doesn't look into the header (although there is a TODO to extract the text track, if it exists). The only possibly shared code would be to extract the size fields (4 * %0xxxxxxx), but formats/mpeg uses BitReader, and I didn't use that for parsing the header (although I guess there's no reason why it can't). "IDentify an MP3" is what ID3 was designed for. I didn't realize that it could be in other formats. According to http://id3.org/Introduction, there is a github library that supports ID3 tags in MP3 and FLAC files (plus MP3 files can have "APE tags", whatever that is). For FLAC we simply check that the first 4 bytes are "fLaC" and don't do anything beyond that, so it won't be an issue. wikipedia notes that these tags can be used in AIFF and WAV files, but it appears that these are nested inside other elements (e.g. in AIFF the tag is stored inside an IFF chunk named "ID3").
On 2017/04/11 at 20:33:58, jrummell wrote: > On 2017/04/10 19:42:23, DaleCurtis wrote: > > Can we reuse media/formats/mpeg for this or extract some common functionality? > > Also is it sufficient to just mark ID3 presence as MP3? Are there any other > > formats which use ID3 that we'd care about? > > I thought about using media/formats/mpeg but there's no dependency allowed from media/base (it's the other way). Plus the mpeg code is only included if proprietary_codecs is set. I looked the code to see what it does, but it appears to simply skip over the header, and doesn't look into the header (although there is a TODO to extract the text track, if it exists). The only possibly shared code would be to extract the size fields (4 * %0xxxxxxx), but formats/mpeg uses BitReader, and I didn't use that for parsing the header (although I guess there's no reason why it can't). > > "IDentify an MP3" is what ID3 was designed for. I didn't realize that it could be in other formats. According to http://id3.org/Introduction, there is a github library that supports ID3 tags in MP3 and FLAC files (plus MP3 files can have "APE tags", whatever that is). For FLAC we simply check that the first 4 bytes are "fLaC" and don't do anything beyond that, so it won't be an issue. wikipedia notes that these tags can be used in AIFF and WAV files, but it appears that these are nested inside other elements (e.g. in AIFF the tag is stored inside an IFF chunk named "ID3"). Okay, so it sounds like we're just worried about MP3's which start with ID3 tags instead of an mpeg packet. Want to reduce the code to just check ID3 presence at start == mp3?
On 2017/04/11 20:49:02, DaleCurtis wrote: > Okay, so it sounds like we're just worried about MP3's which start with ID3 tags > instead of an mpeg packet. Want to reduce the code to just check ID3 presence at > start == mp3? Good idea. Let's make it really simple (and the code smaller). Done.
Description was changed from ========== Handle ID3 tags when checking for MP3 format Some MP3 files contain a image in the ID3 header of MP3 files. As DetermineContainer() only checks the first 8K of a file trying to determine what type of container is being used, check what's in the header as well as any audio data found. BUG=709163 TEST=added files to MP3 test case ========== to ========== Simplify checking for MP3 format Assume that anything that starts with "ID3" is a MP3 format and don't bother validating the content looking for audio frames. BUG=709163 TEST=added files to MP3 test case ==========
Updated the CL description as well.
lgtm assuming the answer to the question is yes :) https://codereview.chromium.org/2807463003/diff/20001/media/base/container_na... File media/base/container_names_unittest.cc (right): https://codereview.chromium.org/2807463003/diff/20001/media/base/container_na... media/base/container_names_unittest.cc:174: TestFile(CONTAINER_MP3, GetTestDataFilePath("bear-audio-10s-CBR-no-TOC.mp3")); These tests failed before this patch right?
Thanks for the review. https://codereview.chromium.org/2807463003/diff/20001/media/base/container_na... File media/base/container_names_unittest.cc (right): https://codereview.chromium.org/2807463003/diff/20001/media/base/container_na... media/base/container_names_unittest.cc:174: TestFile(CONTAINER_MP3, GetTestDataFilePath("bear-audio-10s-CBR-no-TOC.mp3")); On 2017/04/11 23:35:45, DaleCurtis wrote: > These tests failed before this patch right? Correct.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491956312377760,
"parent_rev": "ecd5b2121d8bc6584234929bd6acbd99e3bd3df1", "commit_rev":
"06147322f84cd6dc908c41224b8e0c93f1da8518"}
Message was sent while issue was closed.
Description was changed from ========== Simplify checking for MP3 format Assume that anything that starts with "ID3" is a MP3 format and don't bother validating the content looking for audio frames. BUG=709163 TEST=added files to MP3 test case ========== to ========== Simplify checking for MP3 format Assume that anything that starts with "ID3" is a MP3 format and don't bother validating the content looking for audio frames. BUG=709163 TEST=added files to MP3 test case Review-Url: https://codereview.chromium.org/2807463003 Cr-Commit-Position: refs/heads/master@{#463894} Committed: https://chromium.googlesource.com/chromium/src/+/06147322f84cd6dc908c41224b8e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/06147322f84cd6dc908c41224b8e...
Message was sent while issue was closed.
https://uma.googleplex.com/p/chrome/histograms/?endDate=latest&dayCount=1&his... https://codereview.chromium.org/2807463003/diff/20001/media/base/container_na... File media/base/container_names.cc (right): https://codereview.chromium.org/2807463003/diff/20001/media/base/container_na... media/base/container_names.cc:1584: case TAG('I','D','3',0): Ah, I think this accidentally completely dropped non-id3 mp3 files.
Message was sent while issue was closed.
Hmm, actually mp3 is still processed fine, not sure why we have a massive unknown jump... but starts with the version this rolled into... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
