Closed Bug 1363647 Opened 3 years ago Closed 2 years ago

Fail to retrieve metadata from a very short mp3

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

(Keywords: compat, testcase-wanted)

Attachments

(2 files, 1 obsolete file)

https://github.com/webcompat/web-bugs/issues/5628



URL: https://music.yandex.ru/artist/1484624
Browser / Version: Firefox 55.0
Operating System: Windows 8.1
Problem type: Video doesn't play

Steps to Reproduce

    Navigate to: https://music.yandex.ru/artist/1484624
    …

Expected Behavior:

Actual Behavior:

Technical Information:
Error Code: NS_ERROR_DOM_MEDIA_METADATA_ERR (0x806e0006)
Resource: data:audio/mp3;base64,//uQZAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWGluZwAAAA8AAAACAAACcQCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICA//////////////////////////////////////////////////////////////////8AAAA8TEFNRTMuOThyBK8AAAAAAAAAADQgJAawTQABzAAAAnGGK6g3AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA//sQRAAP8AAAf4AAAAgAAA/wAAABAAAB/gAAACAAAD/AAAAETEFNRTMuOTguMlVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVX/+xBkIg/wAABpAAAACAAADSAAAAEAAAGkAAAAIAAANIAAAARVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVQ==

From webcompat.com with ❤️
Assignee: nobody → ayang
Attached audio test.mp3
This is the mp4 file after base64 decoding.
(In reply to Alfredo Yang (:alfredo) from comment #1)
> Created attachment 8866213 [details]
> test.mp3
> 
> This is the mp4 file after base64 decoding.

Well, I mean mp3...
Ok, I found the problem, the mp3 demuxer needs to read more data in case of the first audio frame is invalid [1].
However, this mp3 is too short to meet this criteria, so it fails to retrieve metadata. Add EOS checking in MP3TrackDemuxer::FindFirstFrame() should be able ot fix this.

[1] Bug 1256590
Priority: -- → P1
Attachment #8866254 - Flags: review?(gsquelart) → review?(jyavenard)
I would prefer Jean-Yves to review this change.
Attachment #8866254 - Flags: review?(jyavenard) → review?(esawin)
Comment on attachment 8866254 [details]
Bug 1363647 - use current current candidate frame for very short mp3 file.

https://reviewboard.mozilla.org/r/137876/#review141480

::: dom/media/MP3Demuxer.cpp:439
(Diff revision 1)
>      mOffset = currentFrame.mEnd;
>      const MediaByteRange prevFrame = currentFrame;
>  
>      // FindNextFrame() here will only return frames consistent with our candidate frame.
>      currentFrame = FindNextFrame();
> +    if (currentFrame.IsEmpty()) {

I don't think that's right.

We should only abort early if there's no frame that follow.

But letting Eugene review it, it's his code.
Comment on attachment 8866254 [details]
Bug 1363647 - use current current candidate frame for very short mp3 file.

https://reviewboard.mozilla.org/r/137876/#review142098

I think this patch will break the tests for "small-shot-false-positive.mp3".

::: commit-message-120d8:1
(Diff revision 1)
> +Bug 1363647 - use current current candidate frame for very short mp3 file. r?gerald

Redundant "current".

::: dom/media/MP3Demuxer.cpp:439
(Diff revision 1)
>      mOffset = currentFrame.mEnd;
>      const MediaByteRange prevFrame = currentFrame;
>  
>      // FindNextFrame() here will only return frames consistent with our candidate frame.
>      currentFrame = FindNextFrame();
> +    if (currentFrame.IsEmpty()) {

We're checking for !currentFrame.Length() next, which is equivalent to currentFrame.IsEmpty(), to reset the candidate frame.

What is the exact issue we're fixing, FindNextFrame failing ungracefully because of EOS?

While we're touching this code, I think some uses of MediaByteRange::Length can be replaced with MediaByteRange::IsEmpty for improved clarity.

::: dom/media/MP3Demuxer.cpp:465
(Diff revision 1)
>                " Length()=%" PRIu64,
>                candidateFrame.mStart, candidateFrame.Length());
>      }
>    }
>  
> -  if (numSuccFrames >= MIN_SUCCESSIVE_FRAMES) {
> +  if (candidateFrame.IsEmpty()) {

Isn't that the wrong constraint? This is supposed to check whether we have successfully found enough consistent frames.
Comment on attachment 8866254 [details]
Bug 1363647 - use current current candidate frame for very short mp3 file.

https://reviewboard.mozilla.org/r/137876/#review142320

Canceling r? due to PTO.
JanH is familiar with this code, he could help out or review in the meanwhile. Otherwise, I can take the review on May 29.
Attachment #8866254 - Flags: review?(esawin)
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8866254 [details]
Bug 1363647 - use current current candidate frame for very short mp3 file.

https://reviewboard.mozilla.org/r/137876/#review142098

> We're checking for !currentFrame.Length() next, which is equivalent to currentFrame.IsEmpty(), to reset the candidate frame.
> 
> What is the exact issue we're fixing, FindNextFrame failing ungracefully because of EOS?
> 
> While we're touching this code, I think some uses of MediaByteRange::Length can be replaced with MediaByteRange::IsEmpty for improved clarity.

Yes, a very short mp3 which is EOS before "numSuccFrames < MIN_SUCCESSIVE_FRAMES" and then returning an empty candidate frame.
I'll update patch to use MediaByteRange::IsEmpty().
Attachment #8866254 - Attachment is obsolete: true
The problem here is a very short mp3 audio and FindNextFrame() returns { 0, 0} at EOS.
However, when FindNextFrame() returns { 0, 0 }, we don't know it is EOS, exceeding MAX_SKIPPED_BYTES or parsing fails.

I think it'd be better to fix it first so demuxer knows it reachs EOS.
Flags: needinfo?(jh+bugzilla)
Hm, so at a guess they're trying to see whether the browser can play MP3 files and of course they're only caring about Chrome's MP3 parser?

In any case I concur, that patch is not the correct solution because it achieves its goal by breaking our false positive detection.

Detecting EOS specifically (as opposed to exceeding MAX_SKIPPED_BYTES) won't help you either - for a file that's not as ridiculously short as the current example, but still at most slightly more than MAX_SKIPPED_BYTES (64 k), we can also hit EOS if our initial candidate frame is a false positive and we fail to find any further matching frame headers within the file.

Actually, if I'm reading the logic (https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/dom/media/MP3Demuxer.cpp#508) right, once we've found a candidate frame and are looking for following frames, we no longer check MAX_SKIPPED_BYTES anyway - maybe something (i.e. honouring MAX_SKIPPED_BYTES during findFirstFrame() as well) to think about for a separate bug?

What you're actually interested in is whether the file was so short that it couldn't even accommodate the required number of successive frames.
I think we could do is check the position of the parser when it gave up after this line (https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/dom/media/MP3Demuxer.cpp#438). If currentFrame.IsEmpty() and the distance between the point where the parser gave up (can be retrieved from mOffset) and the end of the last detected frame (prevFrame.mEnd) is 0 (I hope I'm not off by one here), i.e. we've hit EOS immediately after the previous frame, we could just accept the candidate frame even if numSuccFrames is only 3.
Of course this logic wouldn't work with files that have some unofficial padding or legitimate metadata (APE, lyrics3, ID3) at the end of the file, which could be of arbitrary size, but properly handling that would open up a rabbit hole that's probably not worth entering for now.


As that kind of special case logic still doesn't sound too nice, though, we could just give in and set MIN_SUCCESSIVE_FRAMES to 3, so we're using the same value as Chrome (https://cs.chromium.org/chromium/src/media/formats/mpeg/mpeg_audio_stream_parser_base.cc?l=356&rcl=c7a432cb37af05b091d065aa7a78f9564ed94a05). You should check that the files from bug 1256590 marked as "broken in ff" (https://bugzilla.mozilla.org/attachment.cgi?id=8730620) still work, but given that Chrome has apparently been using this value for some time, it should be fine.
If we're actually encountering some problems that can only be solved by increasing MIN_SUCCESSIVE_FRAMES back to some higher value, we could then always think about adding any special logic for this kind of test file.
I think one of the issue we have here is that we have the assumption that we need to find where the MP3 data starts should the start of the data be truncated or garbage.

I can understand that we would care about this if we wanted to be able to play a stream that is randomly started at any point.

However, with HTML5 this is never going to happen, or at least we can assume that it will never happen. Data can be assumed to always start at index 0.

If we take the assumption that if we have an MP3 content, then the valid data starts right away, then we don't need to worry about finding where the data starts.

From that point on, we can say we have a valid MP3 *if* we find X frames or we hit EOS while attempting to find X frames.

The concept of MAX_SKIPPED_BYTES becomes irrelevant.
I'm not absolutely sure I can follow you 100% re "with HTML5 this is never going to happen, or at least we can assume that it will never happen. Data can be assumed to always start at index 0.", but didn't
- bug 1256590 show that we *do* encounter MP3 streams that randomly start in the middle of a frame
- you yourself suggest in bug 1194085 comment 4 that we shouldn't keep going until EOS (which could entail downloading a multi-megabyte file) if we cannot find any frames?
(In reply to Jan Henning [:JanH] from comment #13)
> I'm not absolutely sure I can follow you 100% re "with HTML5 this is never
> going to happen, or at least we can assume that it will never happen. Data
> can be assumed to always start at index 0.", but didn't
> - bug 1256590 show that we *do* encounter MP3 streams that randomly start in
> the middle of a frame

I think it's fair to treat those as being corrupted file (error).

> - you yourself suggest in bug 1194085 comment 4 that we shouldn't keep going
> until EOS (which could entail downloading a multi-megabyte file) if we
> cannot find any frames?

Above I suggested two things:

1- Valid data must start right away (so MAX_SKIPPED_BYTES is no longer required)
2- That we use min(EOS_POS, MIN_SUCCESSIVE_FRAMES) instead. If EOS is encountered before MIN_SUCCESSIVE_FRAMES is hit, then we assume the file is valid.

Now that may be a silly idea, but doing 2) would get through this bug no?
(In reply to Jean-Yves Avenard [:jya] from comment #14)
> (In reply to Jan Henning [:JanH] from comment #13)
> > I'm not absolutely sure I can follow you 100% re "with HTML5 this is never
> > going to happen, or at least we can assume that it will never happen. Data
> > can be assumed to always start at index 0.", but didn't
> > - bug 1256590 show that we *do* encounter MP3 streams that randomly start in
> > the middle of a frame
> 
> I think it's fair to treat those as being corrupted file (error).

Except of course that those files will still play in Chrome and other browsers or media players, so from a user perspective Firefox will simply appear broken.

> > - you yourself suggest in bug 1194085 comment 4 that we shouldn't keep going
> > until EOS (which could entail downloading a multi-megabyte file) if we
> > cannot find any frames?
> 
> Above I suggested two things:
> 
> 1- Valid data must start right away (so MAX_SKIPPED_BYTES is no longer
> required)
> 2- That we use min(EOS_POS, MIN_SUCCESSIVE_FRAMES) instead. If EOS is
> encountered before MIN_SUCCESSIVE_FRAMES is hit, then we assume the file is
> valid.
> 
> Now that may be a silly idea, but doing 2) would get through this bug no?

Probably yes, but just adjusting MIN_SUCCESSIVE_FRAMES to 3 (3 successive false positive "frame headers" with just the right alignment should be still pretty unlikely) to match Chrome is even simpler and doesn't require making the parser more restrictive with regards to improperly cut files.
Comment on attachment 8868036 [details]
Bug 1363647 - reduce MIN_SUCCESSIVE_FRAMES to 3 for a very short mp3 file.

https://reviewboard.mozilla.org/r/139622/#review142944

Seems fine to me, just needs a short comment explaining why we've now ended up with this particular value.

::: dom/media/MP3Demuxer.cpp:423
(Diff revision 1)
>  }
>  
>  MediaByteRange
>  MP3TrackDemuxer::FindFirstFrame()
>  {
> -  static const int MIN_SUCCESSIVE_FRAMES = 4;
> +  static const int MIN_SUCCESSIVE_FRAMES = 3;

Please add a comment explaining that
- some websites use test files that are actually that short
- we're now using the same value as Chrome
Attachment #8868036 - Flags: review?(jh+bugzilla) → review+
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7222294d9692
reduce MIN_SUCCESSIVE_FRAMES to 3 for a very short mp3 file. r=JanH
https://hg.mozilla.org/mozilla-central/rev/7222294d9692
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Duplicate of this bug: 1365315
Duplicate of this bug: 1375797
You need to log in before you can comment on or make changes to this bug.