1168435 - Replace MP3FrameParser with new FrameParser

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Eugen Sawin [:esawin]]

We should replace the current MP3FrameParser with the FrameParser from bug 1166779 to avoid redundancy.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

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

Comment 2

[Eugen Sawin [:esawin]]

(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.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks! That's good to hear.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1168435-Increase-strictness-of-ID3-header-detect.patch

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.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

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;

Comment 6

[Eugen Sawin [:esawin]]

> 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.

Comment 7

[Eugen Sawin [:esawin]]

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

Moving patch out into bug 1191281

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1168435-Refactor-new-MP3-frame-parser-out-of-the.patch

The first step is to split the new frame parser out of the new demuxer.

Comment 9

[Eugen Sawin [:esawin]]

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.

Comment 10

[Jean-Yves Avenard [:jya]]

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

Comment 11

[Eugen Sawin [:esawin]]

> 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.

Comment 12

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1168435-Split-MP3TrackDemuxer-from-MP3Demuxer.patch

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).

Comment 13

[Eugen Sawin [:esawin]]

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.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1168435-GStreamReader-replace-MP3FrameParser-usa.patch

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1168435-AppleMP3Reader-replace-MP3FrameParser-us.patch

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1168435-DirectShowReader-replace-MP3FrameParser-.patch

Comment 17

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1168435-DirectShowReader-replace-MP3FrameParser-.patch

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 19

[Jan Henning [:JanH]]

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?

Comment 20

[Jean-Yves Avenard [:jya]]

it is indeed and it should be removed (I thought it had already). I'll open a bug to remove it.

Comment 21

[Jan Henning [:JanH]]

GStreamReader and AppleMP3Reader have already disappeared and the DirectShowReader will soon be gone, too, which simplifies this quite a bit.

Comment 22

[Eugen Sawin [:esawin]]

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 23

[Jan Henning [:JanH]]

Attachment: Bug 1168435 - Part 1 - Remove old MP3FrameParser.

All former users of the old MP3 parsing code are gone, so we can now just remove the parser itself as well.

We also need to fix up an include issue in AutoTaskQueue.h that was previously masked by MP3FrameParser.cpp through unified builds.

Review commit: https://reviewboard.mozilla.org/r/145850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145850/

Comment 24

[Jan Henning [:JanH]]

Attachment: Bug 1168435 - Part 2 - Refactor new MP3 frame parser out of the demuxer.

Review commit: https://reviewboard.mozilla.org/r/147778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147778/

Comment 25

[Jan Henning [:JanH]]

Attachment: Bug 1168435 - Part 3 - Split MP3TrackDemuxer from MP3Demuxer.

Review commit: https://reviewboard.mozilla.org/r/147780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147780/

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8876835 [details]
Bug 1168435 - Part 1 - Remove old MP3FrameParser.

https://reviewboard.mozilla.org/r/145850/#review153060

Comment 27

[Jean-Yves Avenard [:jya]]

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

Comment 28

[Jean-Yves Avenard [:jya]]

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.

Comment 29

[Jan Henning [:JanH]]

Attachment: Bug 1168435 - Part 3 - Remove separate MP3 name space.

Review commit: https://reviewboard.mozilla.org/r/149482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149482/

Comment 30

[Jan Henning [:JanH]]

Comment on attachment 8876835 [details]
Bug 1168435 - Part 1 - Remove old MP3FrameParser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145850/diff/1-2/

Comment 31

[Jan Henning [:JanH]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147778/diff/1-2/

Comment 32

[Jan Henning [:JanH]]

Comment on attachment 8878880 [details]
Bug 1168435 - Part 3 - Remove separate MP3 name space.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149482/diff/1-2/

Comment 33

[Jean-Yves Avenard [:jya]]

Comment on attachment 8878880 [details]
Bug 1168435 - Part 3 - Remove separate MP3 name space.

https://reviewboard.mozilla.org/r/149482/#review154798

Comment 34

[Mozilla Autoland]

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 35

[Jan Henning [:JanH]]

Comment on attachment 8876835 [details]
Bug 1168435 - Part 1 - Remove old MP3FrameParser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145850/diff/2-3/

Comment 36

[Jan Henning [:JanH]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147778/diff/2-3/

Comment 37

[Jan Henning [:JanH]]

Comment on attachment 8878880 [details]
Bug 1168435 - Part 3 - Remove separate MP3 name space.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149482/diff/2-3/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
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

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/134c66e88b66
https://hg.mozilla.org/mozilla-central/rev/d234d0f6c68b
https://hg.mozilla.org/mozilla-central/rev/505412c61184