Closed
Bug 1203217
Opened 9 years ago
Closed 9 years ago
Extend MP3 demuxer test coverage and logging for files with large ID3v2 tags
Categories
(Core :: Audio/Video: Playback, defect, P5)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: JanH, Assigned: JanH)
References
Details
Attachments
(2 files, 4 obsolete files)
187.54 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
9.96 KB,
patch
|
JanH
:
review+
|
Details | Diff | Splinter Review |
Bug 1197985 fixed the problem whereby ID3v2 tags larger than the parsing buffer weren't properly skipped, leaving the MPEG frame parser vulnerable to erroneously detecting frame sync within the ID3 tag. We should extend the existing gtest to include an MP3 file which trips up the original implementation and also possibly add a bit of logging when we skip an ID3 tag.
Comment 1•9 years ago
|
||
That's an excellent idea. Our team doesn't have time immediately, so setting priority to match until we have immediate feature work out of the way.
Priority: -- → P5
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8660919 -
Flags: review?(kinetik)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8660921 -
Flags: review?(kinetik)
Assignee | ||
Comment 4•9 years ago
|
||
Run the gtests for Part 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b54f9ee477
Assignee | ||
Comment 5•9 years ago
|
||
Sorry, fixed a typo.
Attachment #8660919 -
Attachment is obsolete: true
Attachment #8660919 -
Flags: review?(kinetik)
Attachment #8660924 -
Flags: review?(kinetik)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8660924 -
Attachment is obsolete: true
Attachment #8660924 -
Flags: review?(kinetik)
Attachment #8660925 -
Flags: review?(kinetik)
Comment 7•9 years ago
|
||
Comment on attachment 8660921 [details] [diff] [review] Part 2 - Add logging for ID3v2 tag detection.patch Review of attachment 8660921 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MP3Demuxer.cpp @@ +630,5 @@ > if (tagSize > remainingBuffer) { > // Skipping across the ID3 tag would take us past the end of the buffer, therefore we > // return immediately and let the calling function handle skipping the rest of the tag. > + MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d, " > + "needing to skip %d bytes past the current buffer", Indentation is off.
Attachment #8660921 -
Flags: review?(kinetik) → review+
Updated•9 years ago
|
Attachment #8660925 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Matthew Gregan [:kinetik] from comment #7) > Comment on attachment 8660921 [details] [diff] [review] > Part 2 - Add logging for ID3v2 tag detection.patch > > Review of attachment 8660921 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MP3Demuxer.cpp > @@ +630,5 @@ > > if (tagSize > remainingBuffer) { > > // Skipping across the ID3 tag would take us past the end of the buffer, therefore we > > // return immediately and let the calling function handle skipping the rest of the tag. > > + MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d, " > > + "needing to skip %d bytes past the current buffer", > > Indentation is off. A few of the other logging macros are suffering from the same problem (which is where I picked it up), I'll fix those as well.
Assignee | ||
Comment 9•9 years ago
|
||
cleans up the indentation for the logging macros, carrying forward r+ from kinetik
Attachment #8660921 -
Attachment is obsolete: true
Attachment #8661300 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•9 years ago
|
||
fix typo in commit message, carrying forward r+ from kinetik
Attachment #8661300 -
Attachment is obsolete: true
Attachment #8661658 -
Flags: review+
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ab7de3b301 https://hg.mozilla.org/integration/mozilla-inbound/rev/dab5c069d6d3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/57ab7de3b301 https://hg.mozilla.org/mozilla-central/rev/dab5c069d6d3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•