Closed Bug 908862 Opened 11 years ago Closed 11 years ago

MP3 file no longer plays with new DirectShow backend

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26
Tracking Flags:
Tracking Status
firefox26 + fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(
URL
)

Details

Attachments

(1 file)

908862-id3-strip.patch
93.50 KB, patch
padenot
: review+
Details | Diff | Splinter Review 
Bug 861693, comment 17:
 Trevor Rowbotham (New to Bugzilla) 2013-08-23 01:23:01 PDT

After landing this bug, it would appear that Windows 7/Vista machines are now using DirectShow to decode mp3 files.  If you attempt to play this file [1] in the latest nightly on a Win 7/Vista machine, it will tell you that the "Video cannot be played because the file is corrupt". The browser console gives the following message: Media resource http://nxt-level.com/music/02.%20everlast%20-%20black%20jesus.mp3 could not be decoded. However, setting media.directshow.enabled = false causes the file to play without any problems.

Last good nightly: 2013-08-13
First bad nightly: 2013-08-14

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c146d402a55f&tochan
ge=3c61cc01a3b1

[1] http://nxt-level.com/music/02.%20everlast%20-%20black%20jesus.mp3
Confirmed this on Windows Vista. WMP still plays the file.
I just tested this on a Windows XP VM and nightly fails to play the file there as well, but WMP on the XP VM plays the file without a problem.
tracking-firefox26: --- → ?
I get this too on Windows 8. It seems DirectShow's MP3 parser can't handle the ID3 tags in this file. If I strip the ID3 tags out the file plays in DirectShow and in Firefox 26, so we can fix this by having DirectShowReader's SourceFilter strip the ID3 tags from the stream.

It looks like we can read the size of the ID3 tag from the header, so it's easy to strip that off the front of the stream:
http://id3.org/id3v2.3.0
OS: Windows Vista → All
Hardware: x86_64 → All
We should also only use DirectShow on WinXP right? So there's a separate bug that we're using it on Win >= 7?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> We should also only use DirectShow on WinXP right? So there's a separate bug
> that we're using it on Win >= 7?

roc, I had also assumed that DirectShow was for XP only, but it seems as though we are prefering DirectShowReader in all cases and falling back to WMFReader if DirectShow is disabled according to the the comment on line 525 in [1].  So yes, I think there are 2 separate bugs here.  1) Windows >= Vista are using DirectShow when they should be using WMF and 2) DirectShow cannot play some MP3 files with ID3 header tags.

cpearce, that seems like an easy enough solution to getting DirectShow to play the MP3 file.  It just seems odd that DirectShow doesn't already handle ID3 tags properly unless the one in that file is malformed in some way.  Are other media players that use DirectShow also forced to strip the ID3 tags?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=784134&action=diff#a/content/media/DecoderTraits.cpp_sec6
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> We should also only use DirectShow on WinXP right? So there's a separate bug
> that we're using it on Win >= 7?

No. We're deliberately using DirectShow for MP3 on all Windows versions. When bug 908503 is fixed, we don't want to be keeping the WMFReader around for just MP3.
Strip the ID3 tags out of the stream that we expose to DirectShow.

I do this by parsing the ID3 headers to get their length, and wrapping the MediaResource with an interface that makes it appear the resource has the header removed. This way I can centralize the logic to translate offsets from the underlying stream to the stream exposed to DirectShow.

https://tbpl.mozilla.org/?tree=Try&rev=43feb2b44298
Attachment #796389 - Flags: review?(paul)
Comment on attachment 796389 [details] [diff] [review]
908862-id3-strip.patch

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

::: content/media/directshow/SourceFilter.cpp
@@ +7,5 @@
>  #include "SourceFilter.h"
>  #include "MediaResource.h"
>  #include "mozilla/RefPtr.h"
>  #include "DirectShowUtils.h"
> +#include "prlog.h"

Is this needed?

@@ +108,5 @@
>  // SourceFilter.
>  //
> +// We can expose only a segment of the MediaResource to the filter graph.
> +// This is used to strip off the ID3 tags from the stream, as DirectShow
> +// has trouble parsing some headers.

Nit: we are dealing with ID3v2, here, which is an entirely different beast from ID3 (v1), maybe it would be good to be more specific.

@@ -192,5 @@
>      mQueriedForAsyncReader(false)
>  {
>    MOZ_COUNT_CTOR(OutputPin);
>    LOG("OutputPin::OutputPin()");
> -  mResource->Seek(nsISeekableStream::NS_SEEK_SET, 0);

I take it this was not needed after all?

@@ +687,5 @@
> +    return NS_OK;
> +  }
> +
> +  int32_t id3Length =
> +    10 + 

trailing space.

@@ +691,5 @@
> +    10 + 
> +    (int32_t(0x7f & header[6]) << 21) +
> +    (int32_t(0x7f & header[7]) << 14) +
> +    (int32_t(0x7f & header[8]) << 7) +
> +     int32_t(0x7f & header[9]);

Yay, third time we have some flavour of mp3 bitstream parsing in the tree!

@@ +714,5 @@
> +      id3TagsEndOffset += length;
> +    } else {
> +      break;
> +    }
> +  }

This calls for a do {} while.

@@ +726,5 @@
>    LOG("SourceFilter::Init()");
>  
> +  // Get the offset of MP3 data in the stream, and pass that into 
> +  // the output pin so that the stream that we present to DirectShow
> +  // does not contain ID3 tags. DirectShow can't properly parse some 

nit: trailing spaces.

::: content/media/test/manifest.js
@@ +181,5 @@
>  
>    { name:"small-shot.m4a", type:"audio/mp4", duration:0.29 },
>    { name:"small-shot.mp3", type:"audio/mpeg", duration:0.27 },
>    { name:"owl.mp3", type:"audio/mpeg", duration:3.29 },
> +  // owl.mp3 as above, but with something funny going on in the ID3 tag

Maybe the funny thing is simply the length? Inspecting the binary in an hex editor, the ID3 container is huge, compared to other mp3 I have on this machine.
Attachment #796389 - Flags: review?(paul) → review+
Thanks for your quick review!

(In reply to Paul Adenot (:padenot) from comment #10)
> Comment on attachment 796389 [details] [diff] [review]
> 908862-id3-strip.patch
> 
> Review of attachment 796389 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/directshow/SourceFilter.cpp
> @@ +7,5 @@
> >  #include "SourceFilter.h"
> >  #include "MediaResource.h"
> >  #include "mozilla/RefPtr.h"
> >  #include "DirectShowUtils.h"
> > +#include "prlog.h"
> 
> Is this needed?

I #def'd on DEBUG_SOURCE_TRACE in SourceFilter.cpp and it took me a while to figure out that the reason I wasn't getting any loggging is because PR_LOGGING wasn't defined, and its definition lies in that prlog.h. So this is to make debugging easier in future.


> 
> @@ +108,5 @@
> >  // SourceFilter.
> >  //
> > +// We can expose only a segment of the MediaResource to the filter graph.
> > +// This is used to strip off the ID3 tags from the stream, as DirectShow
> > +// has trouble parsing some headers.
> 
> Nit: we are dealing with ID3v2, here, which is an entirely different beast
> from ID3 (v1), maybe it would be good to be more specific.

Good point, thanks!

> 
> @@ -192,5 @@
> >      mQueriedForAsyncReader(false)
> >  {
> >    MOZ_COUNT_CTOR(OutputPin);
> >    LOG("OutputPin::OutputPin()");
> > -  mResource->Seek(nsISeekableStream::NS_SEEK_SET, 0);
> 
> I take it this was not needed after all?

Not if we're using MediaResource::ReadAt(). ;)


> ::: content/media/test/manifest.js
> @@ +181,5 @@
> >  
> >    { name:"small-shot.m4a", type:"audio/mp4", duration:0.29 },
> >    { name:"small-shot.mp3", type:"audio/mpeg", duration:0.27 },
> >    { name:"owl.mp3", type:"audio/mpeg", duration:3.29 },
> > +  // owl.mp3 as above, but with something funny going on in the ID3 tag
> 
> Maybe the funny thing is simply the length? Inspecting the binary in an hex
> editor, the ID3 container is huge, compared to other mp3 I have on this
> machine.

Could be. The files that were failing reported in bug 909111 had ID3v2 headers of length 143240 bytes.
https://hg.mozilla.org/mozilla-central/rev/277dee18fec9
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox26: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
I'm not sure if this is completely fixed.  I have another MP3 [1] that has the same problems as the original file in the bug report.  I don't know if this also suffers from the ID3 header problem or if this is a different problem.  I don't know if I am just finding MP3's that are all edge cases or if the files were improperly encoded. I also have another MP3 file where the duration is incorrectly reported with the DirectShow backend, but I will file a new bug for that one.

[1] http://nxt-level.com/music/DJ%20Shadow%20feat.%20Mos%20Def%20-%20Six%20Days.mp3
(In reply to Paul Adenot (:padenot) from comment #10)
> Maybe the funny thing is simply the length? Inspecting the binary in an hex
> editor, the ID3 container is huge, compared to other mp3 I have on this
> machine.

Seems like experts agree with this hypothesis:

http://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/63084404-ca84-4a3c-ab20-6972b5fb60c7/directshow-reports-error-80040218-no-combination-of-filters-could-be-found-to-render-the-stream
(In reply to Trevor Rowbotham from comment #14)
> I'm not sure if this is completely fixed.  I have another MP3 [1] that has
> the same problems as the original file in the bug report.

Thanks for reporting this, I filed bug 910996 to track solving this.
I'm getting this same error on an Ubuntu 12.04 installation of Firefox. Version info is: 27.0.1 "Mozilla Firefox 1.0 for Ubuntu Canonical 1.0"
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: