Replace MP3FrameParser with new FrameParser

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: esawin, Assigned: JanH)

Tracking

41 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox56 fixed)

Details

Attachments

(3 attachments, 8 obsolete attachments)

59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
Reporter

Description

4 years ago
We should replace the current MP3FrameParser with the FrameParser from bug 1166779 to avoid redundancy.
Hmm, it looks like the new ID3Parser is slightly more forgiving than what I created in bug 949036. Perhaps the new code is so different that that bug can't reoccur, but we should make sure the samples from that bug ([1], [2]) don't regress.

[1] http://easyeartraining.com/example.mp3
[2] http://easyeartraining.com/example_notags.mp3
Reporter

Comment 2

4 years ago
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #1)
> Hmm, it looks like the new ID3Parser is slightly more forgiving than what I
> created in bug 949036. Perhaps the new code is so different that that bug
> can't reoccur, but we should make sure the samples from that bug ([1], [2])
> don't regress.
> 
> [1] http://easyeartraining.com/example.mp3
> [2] http://easyeartraining.com/example_notags.mp3

Thanks for the note, you're right about it being less strict and I'm planning on incorporating the changes from your fix in this bug (see https://bugzilla.mozilla.org/show_bug.cgi?id=1093815#c26).
However, it does not regress on the linked samples, the bug 1093815 implementation is more strict than the original MP3FrameParser check.
Thanks! That's good to hear.
Reporter

Updated

4 years ago
Assignee: nobody → esawin
Depends on: 1093815
Reporter

Updated

4 years ago
Depends on: 1172419
Reporter

Updated

4 years ago
No longer depends on: 1172419
Blocks: 1186804
Reporter

Comment 4

4 years ago
This patch ports the strictness of the ID3 header detection from bug 949036 to the new frame parser.

We can look into considering extended headers to increase efficiency by larger skips, too, but since validation and parsing is separated in the new parser, that's not directly tied to the detection.
Attachment #8643252 - Flags: review?(emanuel.hoogeveen)
Comment on attachment 8643252 [details] [diff] [review]
0001-Bug-1168435-Increase-strictness-of-ID3-header-detect.patch

Review of attachment 8643252 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: dom/media/MP3Demuxer.cpp
@@ +1035,5 @@
> +    case 3:
> +      return MajorVersion() >= id3_header::MIN_MAJOR_VER &&
> +             MajorVersion() <= id3_header::MAX_MAJOR_VER;
> +    case 4:
> +      return MinorVersion() < 0xFF;

This is less strict than what I had - to my knowledge all existing specs have the minor version set to 0, so I just checked for that. Only 0xFF is actually invalid per spec however, so I could go either way on it.

@@ +1038,5 @@
> +    case 4:
> +      return MinorVersion() < 0xFF;
> +    case 5:
> +      // Validate flags for supported versions, see bug 949036.
> +      return (c & 0xFF >> MajorVersion()) == 0;

Nit: I find it hard to remember the operator precedence of & and >> at a glance, so perhaps the following would be clearer? Up to you.

  return ((0xFF >> MajorVersion()) & c) == 0;
Attachment #8643252 - Flags: review?(emanuel.hoogeveen) → review+
Reporter

Comment 6

4 years ago
> This is less strict than what I had - to my knowledge all existing specs
> have the minor version set to 0, so I just checked for that. Only 0xFF is
> actually invalid per spec however, so I could go either way on it.

Since revisions are backwards compatible, it felt unnecessary to put a constrain on that. From a practical standpoint, however, I agree that adding the constrain would further decrease the chance for false positives (but might also increase false negatives in the future).


> Nit: I find it hard to remember the operator precedence of & and >> at a
> glance, so perhaps the following would be clearer? Up to you.
> 
>   return ((0xFF >> MajorVersion()) & c) == 0;

Agreed, better be explicit about precedence when it's not obvious.
Status: NEW → ASSIGNED
Reporter

Updated

4 years ago
Depends on: 1191281
Reporter

Comment 7

4 years ago
Comment on attachment 8643252 [details] [diff] [review]
0001-Bug-1168435-Increase-strictness-of-ID3-header-detect.patch

Moving patch out into bug 1191281
Attachment #8643252 - Attachment is obsolete: true
Reporter

Comment 8

4 years ago
The first step is to split the new frame parser out of the new demuxer.
Reporter

Comment 9

4 years ago
Comment on attachment 8644492 [details] [diff] [review]
0001-Bug-1168435-Refactor-new-MP3-frame-parser-out-of-the.patch

The "New" filename suffix can be removed once we get rid of the original MP3FrameParser.
Attachment #8644492 - Flags: review?(jyavenard)
Comment on attachment 8644492 [details] [diff] [review]
0001-Bug-1168435-Refactor-new-MP3-frame-parser-out-of-the.patch

Review of attachment 8644492 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have to call this "MP3FrameParserNew.{cpp,h}"

what's the aim of this patch?

I can't really tell.

I'm guessing it's just moving things around
Attachment #8644492 - Flags: review?(jyavenard) → review+
Reporter

Comment 11

4 years ago
> Do we have to call this "MP3FrameParserNew.{cpp,h}"
> 
> what's the aim of this patch?

We move FrameParser out of MP3Demuxer.{cpp,h} to make it available independently. The "New" suffix is only temporary and will go away (in this bug) when we replace the old MP3FrameParser, as currently there is a file name conflict with that one.

The next step is to replace all uses of the old MP3FrameParser with the new one, make all required adjustments to make it work and finally remove the old parser.
Blocks: 1194079
Reporter

Comment 12

4 years ago
Next, we split the MP3TrackDemuxer from the MP3Demuxer, because we'll be reusing MP3TrackDemuxer throughout the code as a replacement for the old MP3FrameParser.

The new MP3 FrameParser is just a dumb parser that doesn't keep any state regarding resource offsets and lengths, so we won't use it directly anywhere but in the MP3TrackDemuxer (for now).
Reporter

Comment 13

4 years ago
The following WIP patches are examples of how we can replace the old parser. This will also reduce code complexity at the caller site a little.
Reporter

Comment 17

4 years ago
Attachment #8647709 - Attachment is obsolete: true
Reporter

Updated

4 years ago
Depends on: 1194080
Component: Audio/Video → Audio/Video: Playback
Mass change P2 -> P3
Priority: P2 → P3
Reporter

Updated

3 years ago
Status: ASSIGNED → NEW
I was looking at giving this the final push and am wondering - is it correct the the DirectShowReader is in fact obsolete as well now that XP is no longer supported?
Flags: needinfo?(jyavenard)
it is indeed and it should be removed (I thought it had already). I'll open a bug to remove it.
Flags: needinfo?(jyavenard)
GStreamReader and AppleMP3Reader have already disappeared and the DirectShowReader will soon be gone, too, which simplifies this quite a bit.
Assignee: esawin → jh+bugzilla
Depends on: 1370192
Attachment #8647707 - Attachment is obsolete: true
Attachment #8647708 - Attachment is obsolete: true
Attachment #8647710 - Attachment is obsolete: true
Reporter

Comment 22

2 years ago
Thanks for taking on this bug, Jan.
It should be much simpler to land this now and we have enough confidence in the "new" parser.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

2 years ago
mozreview-review
Comment on attachment 8876835 [details]
Bug 1168435 - Part 1 - Remove old MP3FrameParser.

https://reviewboard.mozilla.org/r/145850/#review153060
Attachment #8876835 - Flags: review?(jyavenard) → review+

Comment 27

2 years ago
mozreview-review
Comment on attachment 8876836 [details]
Bug 1168435 - Part 2 - Refactor new MP3 frame parser out of the demuxer.

https://reviewboard.mozilla.org/r/147778/#review153062

not sure there's much point in splitting the demuxer code, but if you think that it helps maintaining the code...

::: dom/media/mp3/MP3FrameParser.h:14
(Diff revision 1)
> -#include "MediaResource.h"
>  #include "mp4_demuxer/ByteReader.h"
> -#include <vector>
>  
>  namespace mozilla {
>  namespace mp3 {

IIUC, the mp3 namespace is no longer required. please add a new patch removing it.

::: dom/media/mp3/MP3FrameParser.cpp:13
(Diff revision 1)
>  #include <inttypes.h>
>  #include <algorithm>
> -#include <limits>
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/EndianUtils.h"

seeing youre creating a new file. may as well sort the headers properly as per coding style
Attachment #8876836 - Flags: review?(jyavenard) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8876837 [details]
Bug 1168435 - Part 3 - Split MP3TrackDemuxer from MP3Demuxer.

https://reviewboard.mozilla.org/r/147780/#review153064

that i see no advantages on, quite the opposite, also that makes for a different structure ro all the other demuxers.
Attachment #8876837 - Flags: review?(jyavenard) → review-
Attachment #8644492 - Attachment is obsolete: true
Attachment #8647701 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8876837 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 33

2 years ago
mozreview-review
Comment on attachment 8878880 [details]
Bug 1168435 - Part 3 - Remove separate MP3 name space.

https://reviewboard.mozilla.org/r/149482/#review154798
Attachment #8878880 - Flags: review?(jyavenard) → review+

Comment 34

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s aaaf12cec5a9 -d 69830ab24d96: rebasing 402459:aaaf12cec5a9 "Bug 1168435 - Part 1 - Remove old MP3FrameParser. r=jya"
merging dom/media/moz.build
rebasing 402460:64b6e36b937c "Bug 1168435 - Part 2 - Refactor new MP3 frame parser out of the demuxer. r=jya"
merging dom/media/moz.build
warning: conflicts while merging dom/media/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

2 years ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/134c66e88b66
Part 1 - Remove old MP3FrameParser. r=jya
https://hg.mozilla.org/integration/autoland/rev/d234d0f6c68b
Part 2 - Refactor new MP3 frame parser out of the demuxer. r=jya
https://hg.mozilla.org/integration/autoland/rev/505412c61184
Part 3 - Remove separate MP3 name space. r=jya
You need to log in before you can comment on or make changes to this bug.