Firefox is not able to play mp4 <video> tags
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Mozilla Firefox |
Fix Released
|
Wishlist
|
|||
firefox (Ubuntu) |
Fix Released
|
Wishlist
|
Unassigned | ||
iceweasel (Debian) |
Fix Released
|
Unknown
|
Bug Description
With firefox 3.5 html5 was introduced. Since 4.0 both ogg/theora and webm/vp8 video formats are supported.
As of firefox 14, gstreamer support can be enabled with --enable-gstreamer. If the required gstreamer codecs are installed then firefox can play H.264 in a <video> tag.
=== Open questions ===
1. Note that the Windows build still only supports patent free codecs.
2. Note that youtube already supports webm/vp8 (altough still not for all videos), while wikipedia supports ogg/theora. Which important sites require other codecs?
3. Chrome supports H.264, but promised to drop support for it ( http://
4. How stable that code is? Which regression could introduce?
5. Given the Ubuntu commitment to free software, do we really want to enable support for other codecs other than the officially supported free ones?
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #2 |
Created an attachment (id=270987)
First stab at impementing <video> element
This patch is a start at implementing the WHATWG <video> element. It successfully plays Ogg Theora videos on Mac, Windows and Linux with some issues which I'm working on.
On Mac OS X videos play in debug ok, but there is no sound in optimized builds. On Windows sound works fine in optimized builds. On Linux (only 64 bit linux tested so far) it plays slowly with no sound.
In all cases on optimized builds Firefox segfaults on exit. Problems also occur if streaming video is played and played over a slow link.
I know about all those issues and am working on them. I'm also using channels some things in thread unsafe ways (like channels) which I am now working on correcting.
But it does play videos for those so here's the first patch attempt for those that want to try it out. So in summary, use a debug until I've corrected these issues over the next few days.
This patch does not include the various Ogg and Portaudio libraries. That file is about 20MB so I've put it separately here: http://
Apply the third party modules patch first, and then this one. Some example usage is available here: http://
You can download the example usage HTML and ogg files: http://
Another thing I plan to add is a configure option to not include the video option.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #3 |
I think probably the configure option(s) should be at the codec level --- i.e. --enable-
In Mozilla Bugzilla #382267, Ivo Emanuel Gonçalves (saoshyant) wrote : | #4 |
While this is still an early stage, I would like to remind that the <audio> works similarly to <video>, so do not forget it as this patch matures. Furthermore, Vorbis and Speex support for <audio> would be great.
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #5 |
Created an attachment (id=272132)
Version 2 of Video patch
This new version of the patch to implement <video> adds/fixes the following:
* Ogg codec support can be enabled/disabled with configure flag --disable-ogg. Currently if the Ogg codec is disabled then the video element is disabled too. In the future if/when other codecs are supported this can change.
* Fix build problems when doing libxul enabled builds
* Fix link error on windows when doing a --disable-libxul build
* Fix colour playback issues on Linux and Windows
* Handle no audio device being present
* Adjust element size when video size information is read from the Ogg file
* No longer use channel across threads
* Various refactorings based on email feedback
A couple of issues still to track down:
1) Sound not working on Linux
2) Sound not working on Mac OS X optimized builds
Remember you need to first apply the third party library patch at:
http://
This has been updated since the last patch for some updates to the libraries.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #6 |
A few comments:
Remove "configure" changes from subsequent patches.
What is MOZ_NO_CONFIG?
-DIRS = base canvas media html xml xul xbl xslt
+DIRS = base canvas html xml xul xbl xslt
What state is your tree in? "media" is not present on trunk.
We'll need to figure out a final destination for portaudio and the Ogg libraries. "content" isn't it.
You should probably start making your printfs #ifdef DEBUG_doublec, or (better) convert them to PR_LOG.
+class VideoInvalidate
+public:
+ VideoInvalidate
There will be problems if the frame goes away while one of these events is pending. Similar issues apply to your other events. There are two ways to solve this:
-- Keep a reference to pending events from the content element or (in this case) the frame, and if the element/frame is destroyed while an event is pending, call a method on the event to disconnect it from the data that's going away. nsRevocableEventPtr can help with that.
-- Hold strong (nsCOMPtr/nsRefPtr) references to content elements from events so the element can't be deleted until the event has fired. You can't hold strong references to frames, since they're not refcounted, but you can hold a reference to the frame's element and simply re-lookup the frame when the event fires, via an nsIPresShell (which you can get from a frame, and also should hold a strong reference to). Of course you have to be aware there might no longer be a frame.
These events need comments explaining what they're for and what thread they run on. In fact some documentation for the general plan here would be good.
It seems that you have threads other than the main thread touching content elements and possibly even frames. This is not good. Any state that needs to be shared across threads should be broken out into its own object. This might be a good time to break out the ogg-playing code into its own object so there's less ogg-specific stuff in nsHTMLVideoElement.
+ virtual ~VideoInvalidat
This is not needed.
+ PresentationEve
You generally do not want to pass nsCOMPtrs as parameters. Just take an nsIThread* here. Also, naming conventions: aAudioStream, etc.
+ mDecoding = false;
PR_FALSE (and PR_TRUE elsewhere, etc)
+ mAudioThread = 0;
nsnull
+ if (mRGB) {
+ PR_Free(mRGB);
+ }
Use nsAutoPtr/
+ unsigned long mFrames;
Use PRUint32 or PRUint64.
+ unsigned char* getVideoRGB();
GetVideoRGB() (similar for other names)
+ bool IsStopped();
+ bool IsDecoding();
+ bool IsPaused();
You should really use PRBool instead of bool, here and elsewhere.
+ NS_IMETHOD Stop();
Don't use NS_IMETHOD unless you're actually implementing an XPCOM interface. You probably just want "virtual void" here.
+class AudioDevice
Public classes like this should use the ns... prefix. Ditto for ChannelToPipeLi
+ ...
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #7 |
One thing that's not implemented yet that's quite important is fallback to the contents of the <video> element when <video> does not support the given media format. You'll have to study how <object> does that...
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #8 |
(In reply to comment #6)
> What is MOZ_NO_CONFIG?
I have no idea. Where does this appear?
>
> What state is your tree in? "media" is not present on trunk.
See the bit in the my comment about the patch requiring the 'third party library' patch to be applied first:
http://
> We'll need to figure out a final destination for portaudio and the Ogg
> libraries. "content" isn't it.
How about in the root 'modules' directory, where libbz2, etc are? If that is the preferred place I'd need some advice on how to have 'content/
> -- Keep a reference to pending events from the content element...
To do this I'll need to keep a collection of events. What's the best way of storing an array/list of events? Is there a collection class that can safely hold nsCOMPtr's?
If the events are posted to the queue of a thread which is not the main thread, is it sufficient to stop those threads - therefore the remaining events won't be processed? That's they way I'm currently handling it.
> It seems that you have threads other than the main thread touching content
> elements and possibly even frames.
All frame stuff is done on the main thread. The only thread code that affects the frame is via an event dispatched to the main thread. I do call GetPrimaryFrame in that non-main thread though, to pass to the invalidate event. I'll move to passing the element to the event and have that call GetPrimaryFrame so it occurs in the main thread. Is that ok?
No element code is touched on other threads except for specific members I added, so I'm pretty sure nothing in the core code is being called off the main thread. I do plan to move that code out into another object though to seperate it from the element.
> You generally do not want to pass nsCOMPtrs as parameters. Just take an
> nsIThread* here.
They are references to nsCOMPtr's and being assigned in the constructor initialisation to another nsCOMPtr. If I make it a raw nsIThread*, but my original pointer is an nsCOMPtr<
void var(nsIThread* baz) { ...store baz in an nsCOMPtr for later use... }
nsCOMPtr<
call_bar(foo);
Can I pass that foo directly or do I need to call a method on nsCOMPtr to get at the pointer?
> You'll need to
> use operator new instead of PR_Malloc, which is also a good thing :-)
I used operator new in patch1 but another commenter via email suggested using PR_Malloc :-) What's a good strategy for protecting against integer overflow here? Given that the video size is from the ogg file, a manipulated ogg could craft an overflow.
> Why are you doing this while painting? Is this the only time you can get the
> video height/width? Can the video height and width change during the stream or
> something?
Yes, the height and width can change during the stream. I get the video height and width when decoding the video frame data. So I'd be doing it at the same time as I send the event that does the invalidate on the frame. Would it be better to do it there (in the nsHTMLVideoElement code, just before I call Invalidate)?
Thanks for the comments, I'll ha...
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #9 |
(In reply to comment #8)
> (In reply to comment #6)
> > What is MOZ_NO_CONFIG?
>
> I have no idea. Where does this appear?
@@ -12371,6 +12274,9 @@ MOZ_NO_
MOZ_NO_
MOZ_NO_
MOZ_NO_FAST_LOAD=
+MOZ_OGG=1
+MOZ_VIDEO=1
+MOZ_NO_CONFIG=
> > What state is your tree in? "media" is not present on trunk.
>
> See the bit in the my comment about the patch requiring the 'third party
> library' patch to be applied first:
>
> http://
Oh I see. I'd assumed that only created new files and didn't patch Mozilla files. It would probably be good to do it that way: one patch that simply creates all files for the non-Mozilla libraries and another patch that patches Mozilla files and creates new Mozilla files.
> > We'll need to figure out a final destination for portaudio and the Ogg
> > libraries. "content" isn't it.
>
> How about in the root 'modules' directory, where libbz2, etc are? If that is
> the preferred place I'd need some advice on how to have 'content/
> have that as a dependancy.
I'm not sure. We should ask bsmedberg on IRC or CC him here.
> > -- Keep a reference to pending events from the content element...
>
> To do this I'll need to keep a collection of events. What's the best way of
> storing an array/list of events? Is there a collection class that can safely
> hold nsCOMPtr's?
Are you sure you need multiple events of the same type pending simultaneously? Which event type(s) would that be? Often it's simpler to just have one event which pulls the "next thing to do" off some internal queue.
> If the events are posted to the queue of a thread which is not the main
> thread, is it sufficient to stop those threads - therefore the remaining
> events won't be processed? That's they way I'm currently handling it.
Remaining events are always processed. I believe thread shutdown waits for all events to be processed before returning. I guess this does work, although it would stop working if we start sharing threads between videos, and it might impose significant delay when we tear down a video element.
> > It seems that you have threads other than the main thread touching content
> > elements and possibly even frames.
>
> All frame stuff is done on the main thread. The only thread code that affects
> the frame is via an event dispatched to the main thread. I do call
> GetPrimaryFrame in that non-main thread though, to pass to the invalidate
> event. I'll move to passing the element to the event and have that call
> GetPrimaryFrame so it occurs in the main thread. Is that ok?
That's a good plan. You will have to deal with revoking the invalidate event when the element goes away (or holding a strong ref to the element from the invalidate event).
> No element code is touched on other threads except for specific members I
> added, so I'm pretty sure nothing in the core code is being called off the
> main thread. I do plan to move that code out into another object though to
> seperate it from the element.
Yeah, you might get away with it, but it still smells bad :-)
> > You generally do not want to pass nsCOMPtrs as parameters. ...
In Mozilla Bugzilla #382267, Timeless-bemail (timeless-bemail) wrote : | #10 |
the modules directory was supposedly closed to new entries, we were trying to kill it.
In Mozilla Bugzilla #382267, Maikmerten-gmx (maikmerten-gmx) wrote : | #11 |
I just want to confirm that the frame dimensions may change during Ogg stream playback. Ogg allows concatenation of Ogg streams, meaning that "cat stream1.ogg stream2.ogg > stream.ogg" will produce a new, valid Ogg stream. This was mostly motivated to allow easy playlist streaming for e.g. web radio (read the data off disk, just push it out into the world).
So the frame dimensions can change, the framerate, the audio channel number, the audio sample rate...
Even codecs may change... if you concatenate an Ogg Vorbis (general purpose audio codec) file with an Ogg Speex (speech codec) file the resulting stream would still be valid Ogg - but I've yet to encounter a playback application that properly handles this. If you only support Ogg Theora + Vorbis (which would by far be the most common and useful combination) that's theoretical speaking anyway ;-)
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #12 |
Created an attachment (id=275741)
Version 3 of the <video> element patch
Remember you need to first apply the third party library patch at:
http://
This has been updated since the last patch for some updates to the libraries, and to remove the mozilla specific updates as recommended by Robert.
This version of the patch addresses the comments from the various people who reviewed it on this bug, and via email. This includes:
* Sound works on Linux, Windows and Mac.
* Moved from using PortAudio to Sydney Audio, which is included as part of liboggplay which we're using anyway.
* Moved decoding stuff out of nsHTMLVideoElement and into a separate object, nsVideoDecoder.
* Ditto with the routines to interface with the audio library.
* Various bug fixes
* Implemented some more of the specification.
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #13 |
Created an attachment (id=275742)
Updated patch with 'configure' file removed
Oops, accidentally included 'configure' in the last patch. This one removes it.
In Mozilla Bugzilla #382267, Shaver (shaver) wrote : | #14 |
Cool work, Chris!
Is there a set of tests that people can run a patched build against, perhaps via mochitest, so that we can help start to iron out annoying configuration-based differences and such?
In Mozilla Bugzilla #382267, Aaronleventhal (aaronleventhal) wrote : | #15 |
Has any thought been put toward alternative content for accessibility? Has a bug been filed? I don't know if there are any consequences for a11y, such as captions. Do the captions have to be part of the video stream? Etc. etc.
For *any* new features we put into Gecko there needs to be some thought put into a11y. The a11y work also needs to block the release that the new feature is going into.
In Mozilla Bugzilla #382267, Ian-hixie (ian-hixie) wrote : | #16 |
There are some things that I still need to look at, but by and large my recommendation would be that we should embed "accessibility" concerns into the video streams themselves. It's not just disabled users who need these tools -- videos need captions for people who have no sound (e.g. who are in an office or in a bar); videos need to be able to play back at different rates for people who don't understand the language well or who want to get through large amounts of video quickly; for many videos, the image part is not critical (e.g. in "talking head" interviews), but people will still want the audio stream (e.g. in an audio-only situation such as a hands-free Web browsing system in a car). We want to make as much of the video available and usable to as many people as possible.
In short, I recommend against thinking of there being a need for "alternative content for accessibility", the problem is really "making videos usable to all users in all situations".
As far as captions go, I recommend making it one of the features accessible from the UA context menu of the video, along with other controls such as "full screen" and selecting alternate audio streams.
In Mozilla Bugzilla #382267, Aaronleventhal (aaronleventhal) wrote : | #17 |
I think that sounds reasonable, if the video format can support it. On the other hand, I'm not a closed captioning or second audio program expert.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #18 |
+MOZ_ARG_
+[ --disable-ogg Disable Ogg Codec support],
+ MOZ_OGG=,
+ MOZ_OGG=1)
We want MOZ_ARG_ENABLE_BOOL so this is disabled by default.
Is the lifecycle of the element and its mDecoder documented somewhere? It's not clear when mDecoder should be created and destroyed, when we start the download, etc. We might need a state machine diagram with transitions. In particular you seem to call ElementAvailable when the element is bound to a document OR when src is set even if it's not in a document.
+ifeq ($(findstring 86,$(OS_TEST)), 86)
+ifneq ($(OS_ARCH),Darwin)
+ifdef HAVE_64BIT_OS
+CSRCS += \
+ x86_64/dsp_mmx.c \
Is this going to break on 64-bit Windows?
+XPIDLSRCS = \
+ nsIAudioStream.idl \
Don't use XPCOM unless either a) we want the interface to be accessible or implementable by non-C++ or b) we want the interface to be *dynamically* pluggable. I don't think either applies in this case, so it should just be plain C++. And we don't need an abstract base class, just a concrete class.
+ /**
+ * Initialize the stream
+ */
Explain the parameters.
+ /**
+ * Write data to the hardware.
+ */
Say something about the format of the samples.
+ nsHTMLVideoElem
This should live with nsHTMLVideoElem
+ /* The OggPlayReader member must be the first item in this
+ class - The OggPlay library requires this class layout */
How about making nsChannelReader inherit from OggPlayReader? Then we can be sure that the layout is correct... Right?
+private:
+ nsCOMPtr<
nsRefPtr. Also, 'private' is superfluous.
+ void handleVideoData(int track_num, OggPlayVideoData* video_data);
+ void handleAudioData(int track_num, OggPlayAudioData* audio_data, int size);
+
+ void openAudioStream();
+ void closeAudioStream();
Leading capital letter. Some comments on this interface would be good too.
+ nsCOMPtr<nsIThread> mPresentationTh
A comment would be useful here...
+ // Lock for accessing members across threads
+ PRLock* mLock;
Can you say exactly what state is protected by the lock? And which fields are accessible by which threads?
+ nsChannelReader* mReader;
WHat's the ownership model for this?
+ volatile PRBool mPlaying;
+ volatile PRBool mDecoding;
+ volatile PRBool mPaused;
PRPackedBool. Do these need to be volatile? If they're accessed by multiple threads they should probably be protected by mLock instead.
+ while (available < 300000 && last_available != available) {
+ last_available = available;
+ PR_Sleep(
What do these magic numbers do? And what is the purpose of this function? It looks weird ... it loops until no data arrives for one second or we have 300,000 bytes? That could be a long time...
+ nsChannelReader * me = (nsChannelReader *)opr;
If you do the inheritance thing I suggested, you could use static_cast<> here.
+ if (mAudioHandle) {
Make this "if (!mAudioHandle) return..."
+ static_
Use "short(
+ mLock(0),
nsnull
+ return mLock ? PR_TRUE : PR_FALSE;
mLock != nsnull
+ while (CanDecode()) {
+ ...
In Mozilla Bugzilla #382267, Annevankesteren+mozilla (annevankesteren+mozilla) wrote : | #19 |
I think the idea is that <video> does not have fallback content like <object>.
In Mozilla Bugzilla #382267, Dao (dao) wrote : | #20 |
Yes. From <http://
Content may be provided inside the video element so that older Web browsers,
which do not support video, can display text to the user informing them of
how to access the video contents. User agents should not show this fallback
content to the user.
... which could be a bad idea, given that vendors are free to support any formats. On the other hand, who would actually make videos available in two or more formats? Real use cases would probably be more like <video src="foo.mov">Get Safari</video>.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #21 |
Ah OK. I guess that idea has been superceded by the use of multiple <source> children to specify alternate formats. That's good. Although the spec has grown considerably since I last looked at it, which isn't so good...
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #22 |
>Don't use XPCOM unless either a) we want the interface to be accessible or
>implementable by non-C++ or b) we want the interface to be *dynamically*
>pluggable. I don't think either applies in this case, so it should just be
>plain C++. And we don't need an abstract base class, just a concrete class.
I used XPCOM to make it easier for other people to implement different audio backends if Sydney wasn't available for their platform. It also made it easier for me to swap PortAudio and Sydney for testing purposes. I can make it plain C++ inheriting from an abstract base class and get the same effect but you're suggesting not to do that as well? Is the preference to have a concrete class with different implementations used via #defining out other platform code and/or different implementation files?
The same question applies to nsVideoDecoder. I was moving towards having an abstract video decoder class, with Ogg being the provided implementation. So if someone wanted a gstreamer, or other platform specific support, they could just implement that. Is it not worth doing that?
>if you do the inheritance thing I suggested, you could use static_cast<> here.
>...
>Use "short(
In some places It's suggested to change to using a C++ cast, in others, to a C style casts. What's the rule for deciding which I should be using so I can get it right first time?
>We're not using std::nothrow elsewhere are we? If not, not much point in
>starting to use it here.
One reviewer suggesting using PR_Malloc to avoid the problem of operator new throwing an exception in an out of memory condition which, since we don't use exception handling anywhere, is likely to be bad. You suggested changing back to operator new. To solve both requests I used nothrow. Has operator new been changed in Mozilla to return 0 in out of memory conditions? Is it something that's not worth worrying about?
Thanks for the comments, I'll make the requested changes.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #23 |
(In reply to comment #22)
> >Don't use XPCOM unless either a) we want the interface to be accessible or
> >implementable by non-C++ or b) we want the interface to be *dynamically*
> >pluggable. I don't think either applies in this case, so it should just be
> >plain C++. And we don't need an abstract base class, just a concrete class.
>
> I used XPCOM to make it easier for other people to implement different audio
> backends if Sydney wasn't available for their platform. It also made it easier
> for me to swap PortAudio and Sydney for testing purposes. I can make it plain
> C++ inheriting from an abstract base class and get the same effect but you're
> suggesting not to do that as well? Is the preference to have a concrete class
> with different implementations used via #defining out other platform code
> and/or different implementation files?
Since we don't need multiple audio backends right now, I would not bother making it easy to add new ones. If someone comes along and wants to add one, they can either extend Sydney (the best option), or add #ifdefs, or patch the code to use an abstract base class instead.
> The same question applies to nsVideoDecoder. I was moving towards having an
> abstract video decoder class, with Ogg being the provided implementation. So
> if someone wanted a gstreamer, or other platform specific support, they could
> just implement that. Is it not worth doing that?
That makes more sense since we know we want to support additional decoders in the future.
> >if you do the inheritance thing I suggested, you could use static_cast<> here.
> >...
> >Use "short(
>
> In some places It's suggested to change to using a C++ cast, in others, to a C
> style casts. What's the rule for deciding which I should be using so I can get
> it right first time?
Use C++ casts wherever you can, which should be almost everywhere; they're safer. I don't recall why or where you needed to use a C-style cast.
> >We're not using std::nothrow elsewhere are we? If not, not much point in
> >starting to use it here.
>
> One reviewer suggesting using PR_Malloc to avoid the problem of operator new
> throwing an exception in an out of memory condition which, since we don't use
> exception handling anywhere, is likely to be bad. You suggested changing back
> to operator new. To solve both requests I used nothrow. Has operator new been
> changed in Mozilla to return 0 in out of memory conditions? Is it something
> that's not worth worrying about?
It hasn't been changed, but it needs to be. I guess you can just leave std::nothrow in for now.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #24 |
Looking at the latest spec, there's a ton of pseudo code there. We should make sure our code is organized so it looks just like the spec, with functions and variables where the spec (implicitly) defines functions and variables.
In Mozilla Bugzilla #382267, Crowderbt (crowderbt) wrote : | #25 |
reinterpret_cast<> should give you anything you would need from a C-style cast, should it not?
In Mozilla Bugzilla #382267, Andre-John Mas (andrejohn-mas) wrote : | #26 |
Are there any plans to support the attributes specified by the spec? From my understanding this should be able to work:
<video src="test4_
The patched version doesn't seem to support this yet?
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #27 |
The patch is not complete yet.
In Mozilla Bugzilla #382267, Ivo Emanuel Gonçalves (saoshyant) wrote : | #28 |
I'm wondering: what audio backend are you using for Linux? ALSA or OSS 4.0? If you are using ALSA, I suggest switching to OSS, as ALSA emulates OSS well and OSS is the default backend of a lot of UNIX and Linux systems.
OSS is also easier to program to.
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #29 |
Ivo, I am using OSS for Linux so hopefully that will cover most systems.
Andre-John, Yes I plan to support the rest of the specification, but the current patch does not support the functionality you mention yet.
Robert, yep, I'm moving to follow the pseudo code closer.
In Mozilla Bugzilla #382267, Henri Sivonen (hsivonen) wrote : | #30 |
Re: comment 16, captioning-related URLs from maikmerten on #whatwg:
http://
http://
http://
(Annodex and Writ are different.)
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #31 |
Created an attachment (id=282950)
Version 5 of the <video> patch
This patch corrects various review comments and bugs. The main change with this patch is rewriting portions of it to follow the pseudo code of the specification. Supported now is the 'autoplay' attribute and the first frame of the video is loaded initially. The element now works embedded as an SVG foreignObject.
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #32 |
Don't forget to apply the third party modules patch first:
http://
Some example usage of the video element here:
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #33 |
+ // Media loading flags. See:
+ // http://
+ PRUint16 mNetworkState;
+ PRUint16 mReadyState;
Can we have typedefs for PRUint16 defined for these?
+ PRBool mBegun;
+ PRBool mLoadedFirstFrame;
+ PRBool mLoadedEnoughTo
+ PRBool mAutoplaying;
+ PRBool mPaused;
+ PRBool mSeeking;
Might as well make these PRPackedBool.
You might want to document any useful invariants that hold between these booleans.
You don't need to initialize the decoder in two different places.
Document the relationship between mAutoplaying and the autoplay attribute, in particular, clarify what the difference is between them.
+ double mVolume;
+#if defined(
+ double mPauseTime;
+#else
+ PRInt64 mPauseBytes;
+#endif
+ void* mAudioHandle;
+ int mRate;
Document the units for all these
The methods of nsVideoDecoder need more documentation to indicate on which thread they are called. Maybe group the methods that are called on each thread? Similarly, group the data members according to which thread they are used by. Maybe even put the thread name in the names of the methods.
+ nsCOMPtr<
+ nsCOMPtr<
+ nsCOMPtr<
Use nsRevocableEventPtr here so you don't have to explicitly Revoke() when these are nulled out in Shutdown()?
+ PRBool mFirstFrameLoaded;
+ PRCondVar* mFirstFrameCondVar;
+ PRLock* mFirstFrameLock;
Explain what these protect and how the condition variable is used.
+ sa_stream_
static_cast (and elsewhere)
+ *aTime = ((double)
+ *aTime = static_
Constructor-
+ nsChannelReader* me = (nsChannelReade
static_cast (and elsewhere)
+ Presentation Thread
+ This thread goes through the data decoded by the decode thread
+ and displays it if it is video, or queues it for playing if it
+ is audio. It owns the audio device - all audio operations occur
+ on this thread.
It doesn't really display it, right? We just queue frames to be picked up by the main thread or something.
I think all the comments here should go in nsVideoDecoder.h.
+ As a final step of the first frame event is to notify a condition
+ variable that we have decoded the first frame.
This doesn't quite parse.
+// An Event that uses an nsVideoDecoder object. It provides a
+// way to inform the event that the decoder object is no longer
+// valid, so queued events can safely ignore it.
+class nsDecoderEvent : public nsRunnable {
How about adding a RunWithLock method that subclasses have to override, so that the "lock ... run ... unlock" pattern of Run() can be shared in the superclass?
Specify which thread each event runs on.
+ if (!mRGB) {
+ mRGBWidth = y_width;
+ mRGBHeight = y_height;
+ if (mRGBWidth < MAX_VIDEO_WIDTH && mRGBHeight < MAX_VIDEO_HEIGHT) {
+ mRGB = new (std::nothrow) unsigned char[mRGBWidth * mRGBHeight * 4];
...
In Mozilla Bugzilla #382267, Chris Double (doublec) wrote : | #34 |
Created an attachment (id=283664)
Version 6 of the <video> element patch
No new functionality from version 5. Addresses the review comments.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #35 |
+ // The following methods must only be called on the main
+ // thread.
Use ***** or something to make these bracketing comments stand out.
+ PRBool mPaused;
+
+ // True if the first frame of data has been loaded. This member,
+ // along with the condition variable and lock is used by threads
+ // that need to wait for the first frame to be loaded before
+ // performing some action. In particular it is used for 'autoplay' to
+ // start playback on loading of the first frame.
+ PRBool mFirstFrameLoaded;
PRPackedBool (and you needn't bother with bitfields, that's a bit more code)
But I think we go for a real review now. I think I can be the reviewer of record for the video and layout parts, and say ... Jonas ... for the content parts.
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #36 |
Jonas, this isn't 1.9 material exactly, but I think a lot of us would like to have this landed on trunk (disabled of course), so I wonder if you'd be interested in reviewing the content/DOM side.
In Mozilla Bugzilla #382267, Brendan-mozilla (brendan-mozilla) wrote : | #37 |
It's fine with me to land this disabled on cvs.mozilla.org, but at some point we should consider using hg.mozilla.org to develop features that could be merged into release vehicles, independently of big-integration
Here is not the place, but now may be the time. Consider this planting a seed. I'll send mail to the relevant lists/groups shortly, but I hope video can blaze a new trail, away from the ever-lovin' CVS repository.
/be
In Mozilla Bugzilla #382267, Jonas-sicking (jonas-sicking) wrote : | #38 |
Honesly, if this is in a state where we're ready to land it on trunk then I think we should attempt to get it in to FF3. This would be a huge help to keep the internet open by promoting open standards for video at a very crucial state for video on the web. Ideally we would have had this in a release a year ago, but waiting another year or two I think would make it much harder for free codecs to get market share.
Yes, I know we're well past feature freeze, but I think this one is worth making an exception for. It speaks directly toward MoFos goal of promoting freedom and innovation on the internet.
In Mozilla Bugzilla #382267, Jonas-sicking (jonas-sicking) wrote : | #39 |
In other words, yes, I'd be willing to review the contet/DOM parts. On my free time if I have to :)
In Mozilla Bugzilla #382267, Roc-ocallahan (roc-ocallahan) wrote : | #40 |
The patch is quite far from being feature-complete with respect to the WHATWG video spec, so I'm still not optimistic about turning it on for FF3. Having it in the tree will make it easier to get help and move it along, of course.
affects: | firefox (Ubuntu) → firefox-3.5 (Ubuntu) |
Changed in firefox-3.5 (Ubuntu): | |
status: | New → Incomplete |
Changed in firefox-3.5 (Ubuntu): | |
status: | Incomplete → New |
Changed in firefox: | |
status: | Unknown → In Progress |
Changed in firefox-3.5 (Ubuntu): | |
importance: | Undecided → Wishlist |
status: | New → Triaged |
Changed in firefox: | |
importance: | Unknown → Wishlist |
402 comments hidden Loading more comments | view all 482 comments |
In Mozilla Bugzilla #422540, Roc-ocallahan (roc-ocallahan) wrote : | #443 |
I think you're ready to land.
In Mozilla Bugzilla #422540, Alessandro Decina (alessandro.decina) wrote : | #444 |
Great, waiting for someone with commit access to merge then \o/
In Mozilla Bugzilla #422540, Michael-monreal+moz (michael-monreal+moz) wrote : | #445 |
As GSteamer 1.0 is just around the corner...
http://
http://
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #446 |
Comment on attachment 615622
GStreamer backend for audio/video decoding (including all the post review fixes)
Chris Double is on vacation. Flagging to land in his stead.
In Mozilla Bugzilla #422540, Alessandro Decina (alessandro.decina) wrote : | #447 |
(In reply to Michael Monreal [:monreal] from comment #275)
> As GSteamer 1.0 is just around the corner...
>
> http://
> http://
> 0.11.txt
Hehe, yeah that's on the todo list already :)
In Mozilla Bugzilla #422540, Ryanvm (ryanvm) wrote : | #448 |
Comment on attachment 615622
GStreamer backend for audio/video decoding (including all the post review fixes)
Ralph, just use the checkin-needed flag. The checkin? flag is mainly used when landing on multiple branches or landing only a subset of patches. Also, obsoleting old patches is appreciated. Also, with mozilla-
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #449 |
Ok, thanks. I just wanted to be clear which patch we intended to land.
In Mozilla Bugzilla #422540, Release-a (release-a) wrote : | #450 |
Try run for 1e0e98d406ac is complete.
Detailed breakdown of the results available here:
https:/
Results (out of 230 total builds):
success: 196
warnings: 34
Builds (or logs if builds failed) available at:
http://<email address hidden>
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #451 |
This is NPOTB (not part of the default build) and should land.
Try push at https:/
In Mozilla Bugzilla #422540, Akeybl (akeybl) wrote : | #452 |
Comment on attachment 615622
GStreamer backend for audio/video decoding (including all the post review fixes)
[Triage Comment]
We've now given NPOTB patches blanket approval - a=npotb. https:/
In Mozilla Bugzilla #422540, Ryanvm (ryanvm) wrote : | #453 |
In Mozilla Bugzilla #422540, Ehsan-mozilla (ehsan-mozilla) wrote : | #454 |
In Mozilla Bugzilla #422540, Dave (fehe) wrote : | #455 |
Keywords field has "mobile" but platform is "all." So, is this still fennec only or should this work with Firefox, including on Windows?
In Mozilla Bugzilla #422540, Alessandro Decina (alessandro.decina) wrote : | #456 |
Should work with Firefox on any platform, including Windows.
In Mozilla Bugzilla #422540, Dao (dao) wrote : | #457 |
(In reply to IU from comment #285)
> Keywords field has "mobile" but platform is "all." So, is this still fennec
> only or should this work with Firefox, including on Windows?
See https:/
The "mobile" keyword means "important to mobile developmental work", not mobile-only.
Changed in firefox: | |
status: | In Progress → Fix Released |
In Mozilla Bugzilla #422540, Konstartyom (konstartyom) wrote : | #458 |
Thank you for your excellent patch. Do you plan to add support of standalone mp3 audio (in the <audio> tag)?
In Mozilla Bugzilla #422540, Diego Viola (diego-viola-deactivatedaccount) wrote : | #459 |
Nice to see this issue got resolved. Thanks to everyone who worked on this.
Does Firefox 12 already has GStreamer support? Or will GStreamer support come with a further Firefox version? Like Firefox 13 or 14?
Thanks.
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #460 |
The patch landed on the Firefox 14 branch. (See the 'Target Milestone' field at the top of the bug). Firefox 14 will become the stable release in July.
Note however that which GStreamer support is in the codebase, Official builds do not have it turned on. At least, not yet.
In Mozilla Bugzilla #422540, Diego Viola (diego-viola-deactivatedaccount) wrote : | #461 |
(In reply to Ralph Giles (:rillian) from comment #290)
> The patch landed on the Firefox 14 branch. (See the 'Target Milestone' field
> at the top of the bug). Firefox 14 will become the stable release in July.
>
> Note however that which GStreamer support is in the codebase, Official
> builds do not have it turned on. At least, not yet.
Thanks a lot.
In Mozilla Bugzilla #422540, A2414578 (a2414578) wrote : | #462 |
Could someone, please, compile a build for Windows with this special flag needed to include GStreamer support?
In Mozilla Bugzilla #422540, Pawsome (pawsome) wrote : | #463 |
(In reply to Ralph Giles (:rillian) from comment #290)
> Note however that which GStreamer support is in the codebase, Official
> builds do not have it turned on. At least, not yet.
Is there a bug for turning it on? If not, should we file one? What are the current plans for this feature?
(In reply to Sean Newman from comment #292)
> Could someone, please, compile a build for Windows with this special flag
> needed to include GStreamer support?
Seconded.
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #464 |
(In reply to Sid from comment #293)
> Is there a bug for turning it on? If not, should we file one? What are the
> current plans for this feature?
If you think it should be on by default you should certainly file a bug so it can be discussed. As far as I know current plans aren't settled; the decision to change policy on the official builds depends on the codec issues, and we don't have consensus or working code there yet.
> (In reply to Sean Newman from comment #292)
> > Could someone, please, compile a build for Windows with this special flag
> > needed to include GStreamer support?
Note that for this to be useful, we'd have to build and include a copy of Gstreamer as well. It's generally available on Linux systems, but not Windows, Mac or Android. That's probably a separate bug as well: getting gstreamer building on non-linux archs either as part of our monolithic build, or by bundling a standalone gstreamer + plugins build, such as the sdk from entropywave, or the one fluendo announced a few months ago.
In Mozilla Bugzilla #422540, Pawsome (pawsome) wrote : | #465 |
(In reply to Ralph Giles (:rillian) from comment #294)
> As far as I know current plans aren't settled; the
> decision to change policy on the official builds depends on the codec
> issues, and we don't have consensus or working code there yet.
Yeah, I forgot about codec problems. Let's wait then. Anyway, if those issues are solved, it would be better to try and implement a native DirectShow or Media Foundation backend for Windows (bug 435339).
> It's generally available on Linux systems, but not Windows, Mac or Android.
There is a Windows port of GStreamer available on http://
In Mozilla Bugzilla #422540, Alessandro Decina (alessandro.decina) wrote : | #466 |
(In reply to Sid from comment #295)
> (In reply to Ralph Giles (:rillian) from comment #294)
> > As far as I know current plans aren't settled; the
> > decision to change policy on the official builds depends on the codec
> > issues, and we don't have consensus or working code there yet.
>
> Yeah, I forgot about codec problems. Let's wait then. Anyway, if those
> issues are solved, it would be better to try and implement a native
> DirectShow or Media Foundation backend for Windows (bug 435339).
Why do you think that would be better?
> > It's generally available on Linux systems, but not Windows, Mac or Android.
>
> There is a Windows port of GStreamer available on
> http://
> Will it work with GStreamer-enabled Nightly build?
GStreamer has pretty good Windows support and it even supports some of the native codecs. Building it and shipping it on Windows is not as easy as it could be though. To fix that, as Ralph mentioned, there's an undergoing effort to produce an easy to use (and ship and build) GStreamer SDK for linux, mac and windows, see http://
I'm waiting for the SDK to be ready (gonna be ready Any Day Now ;)), then my plan is to integrate it in the build and start a discussion on how and where to enable the gstreamer backend by default.
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #467 |
(In reply to Alessandro Decina from comment #296)
> I'm waiting for the SDK to be ready (gonna be ready Any Day Now ;)), then my
> plan is to integrate it in the build and start a discussion on how and where
> to enable the gstreamer backend by default.
Great! Can you open a bug for that? The discussion thread here is long enough already. :)
affects: | firefox-3.5 (Ubuntu) → firefox (Ubuntu) |
shawnlandden (shawnlandden) wrote : | #468 |
The upstream fix is preffed off by default, can we get --enable-gstreamer in the ubuntu builds? (version 15+)
In Mozilla Bugzilla #422540, Pawsome (pawsome) wrote : | #469 |
(In reply to Alessandro Decina from comment #296)
> I'm waiting for the SDK to be ready (gonna be ready Any Day Now ;)), then my
> plan is to integrate it in the build and start a discussion on how and where
> to enable the gstreamer backend by default.
Just in case, SDK is out.
http://
In Mozilla Bugzilla #422540, Cpearce-t (cpearce-t) wrote : | #470 |
(In reply to Sid from comment #298)
> Just in case, SDK is out.
> http://
In the Windows installation instructions they recommend removing the client app's dependency on MSVC2010's runtime DLL and using the "“basic” C runtime which comes in every Windows system since Windows XP, and is named MSVCRT.DLL".
See:
"Removing the dependency with the Visual Studio runtime"
http://
We apparently ship with the MSVC 2010's RT, and switching runtimes makes me nervous... Were we to ship this on Windows, we may be best to build the SDK ourselves with MSVCRT2010 or get Collabora to build one with MSVCRT2010.
In Mozilla Bugzilla #422540, Andoni Morales (ylatuya) wrote : | #471 |
There are several reasons why we decided to build the SDK against the system CRT msvcrt.dll, but the most important one is that if you decide to link against any of the VS ones you will be forced to distribute broken software.
According to MS EULA you can't distribute yourself this "system library" (eg: msvcr100.dll) so your software is depending on a third party installer (Microsoft Visual C++ 20XX Redistributable Package). The GPL also forbids explicitely the redistribution of System Libraries (http://
How is this handled in Firefox?
There is the option of rebuilding the SDK linking against msvcr100.dll, this would require on our side providing a gcc spec that links against moldnames100 and msvcr100 and rebuilding gcc so that libgcc_s_sjlj-1.dll and libstdc++6.dll are linked against the new CRT and using the gcc spec in the toolchain.
In Mozilla Bugzilla #422540, Ted Mielczarek (ted-mielczarek) wrote : | #472 |
We ship the CRT files alongside our app. I don't think your reading of the EULA is correct. Those files are explicitly listed as "redistributable", and historically you have been allowed to ship the DLL files with your application.
The GPL wrinkle is tricky, that sounds like a big PITA. In any event, we ship Firefox under the MPL, so it's not an issue for us.
In Mozilla Bugzilla #422540, Andoni Morales (ylatuya) wrote : | #473 |
(In reply to Ted Mielczarek [:ted] from comment #301)
> We ship the CRT files alongside our app. I don't think your reading of the
> EULA is correct. Those files are explicitly listed as "redistributable", and
> historically you have been allowed to ship the DLL files with your
> application.
Apparently I was hitten by this bug when I last read the EULA: http://
In Mozilla Bugzilla #422540, xunxun1982 (xunxun1982) wrote : | #474 |
Can we introduce GStreamer to Mozilla source tree?
Because on Windows, Gstreamer need rebuilding using the user's compiler.
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #475 |
(In reply to xunxun from comment #303)
> Can we introduce GStreamer to Mozilla source tree?
If you think that would be useful please open a separate bug for discussion. Especially if you can provide patches.
A simpler approach in the near term might be to add support for linking against one of the pre-built SDKs, e.g. from entropywave or fluendo. IIRC fluendo's had a problem with mismatched runtimes on windows, though.
>
> Because on Windows, Gstreamer need rebuilding using the user's compiler.
description: | updated |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
sam tygier (samtygier) wrote : | #476 |
"5. Given the Ubuntu commitment to free software, do we really want to enable support for other codecs other than the officially supported free ones?"
building with gstreamer does not in itself allow firefox to play h264. it means that firefox can use the gstreamer codecs that you can install. this work equivient to rhythmbox, banshee, totem etc, where codec support is expandable by installing new gstreamer codecs.
If there is a desire to prevent firefox supporting non-free formats then its plugin support would need to be disabled so that it can't use flash or realplayer.
Oibaf (oibaf) wrote : | #477 |
Yes, but the only pratical point of adding gstreamer is adding support for H.264, since all the free formats are already natively supported (while other formats aren't interesting in firefox). Other softwares need gstreamer because is the only supported backend also for the free formats.
Also, while flash is widely used, HTML5 video still is not, and there is still not a winner between H.264/Webm, so there is a point in supporting only free formats here - at least until/if H.264 definitively wins over Webm.
sam tygier (samtygier) wrote : | #478 |
having trouble with launchpad, but probably interesting to look at http://
In Mozilla Bugzilla #422540, Gquigs+bugs (gquigs+bugs) wrote : | #479 |
Are there plans to enable this in Linux builds? Should that be a separate Mozilla bug?
In Mozilla Bugzilla #422540, Riles (riles) wrote : | #480 |
There are no plans to enable this in official builds. If you think there should be, please open a new bug for that, and mark it dependent on this one.
In Mozilla Bugzilla #422540, xunxun1982 (xunxun1982) wrote : | #481 |
(In reply to Ralph Giles (:rillian) from comment #304)
> (In reply to xunxun from comment #303)
>
> > Can we introduce GStreamer to Mozilla source tree?
>
> If you think that would be useful please open a separate bug for discussion.
> Especially if you can provide patches.
>
> A simpler approach in the near term might be to add support for linking
> against one of the pre-built SDKs, e.g. from entropywave or fluendo. IIRC
> fluendo's had a problem with mismatched runtimes on windows, though.
>
> >
> > Because on Windows, Gstreamer need rebuilding using the user's compiler.
I found that Opera had a Gstreamer project for windows Clone : http://
sam tygier (samtygier) wrote : | #482 |
there is a more focused discussion on Bug #1051559
Changed in firefox (Ubuntu): | |
status: | Triaged → Fix Released |
Changed in iceweasel (Debian): | |
status: | Unknown → Fix Released |
*** Bug 381758 has been marked as a duplicate of this bug. ***