Ubuntu

Firefox is not able to play mp4 <video> tags

Reported by Kai Mast on 2009-08-12
86
This bug affects 15 people
Affects Status Importance Assigned to Milestone
Mozilla Firefox
Fix Released
Wishlist
firefox (Ubuntu)
Wishlist
Unassigned

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://blog.chromium.org/2011/01/html-video-codec-support-in-chrome.html ).
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?

*** Bug 381758 has been marked as a duplicate of this bug. ***

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://www.double.co.nz/video_test/third_party_modules.patch.gz

Apply the third party modules patch first, and then this one. Some example usage is available here: http://www.double.co.nz/video_test

You can download the example usage HTML and ogg files: http://www.double.co.nz/video_test/video_test.tar.gz

Another thing I plan to add is a configure option to not include the video option.

I think probably the configure option(s) should be at the codec level --- i.e. --enable-theora/--disable-theora. And, if no codecs are enabled, all the support code for the video element should be disabled. Something similar for audio.

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.

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://www.double.co.nz/video_test/third_party_modules.patch.gz

This has been updated since the last patch for some updates to the libraries.

Download full text (4.5 KiB)

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 VideoInvalidateEvent : public nsRunnable {
+public:
+ VideoInvalidateEvent(nsIFrame* f) : mFrame(f) {}

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 ~VideoInvalidateEvent() { }

This is not needed.

+ PresentationEvent(nsHTMLVideoElement* element, nsCOMPtr<nsIThread>& decodeThread, OggPlay* player, PaStream* audio_stream, double framerate, double lastVideoFrameTime) :

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/nsAutoArrayPtr to avoid manual freeing of buffers. You'll need to use operator new instead of PR_Malloc, which is also a good thing :-)

+ 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 ChannelToPipeListener and OggPlayChannelReader.

+ ...

Read more...

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

Download full text (3.2 KiB)

(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://www.double.co.nz/video_test/third_party_modules.patch.gz

> 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/media/video' have that as a dependancy.

> -- 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<nsIThread>, is it safe to do:

  void var(nsIThread* baz) { ...store baz in an nsCOMPtr for later use... }
  nsCOMPtr<nsIThread> foo = ...;
  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...

Read more...

Download full text (5.6 KiB)

(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_ACTIVEX_SUPPORT=1
 MOZ_NO_INSPECTOR_APIS=
 MOZ_NO_XPCOM_OBSOLETE=
 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://www.double.co.nz/video_test/third_party_modules.patch.gz

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/media/video'
> 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. ...

Read more...

the modules directory was supposedly closed to new entries, we were trying to kill it.

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

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://www.double.co.nz/video_test/third_party_modules.patch.gz

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.

Created an attachment (id=275742)
Updated patch with 'configure' file removed

Oops, accidentally included 'configure' in the last patch. This one removes it.

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?

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.

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.

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.

Download full text (4.5 KiB)

+MOZ_ARG_DISABLE_BOOL(ogg,
+[ --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.

+ nsHTMLVideoElement.h \

This should live with nsHTMLVideoElement.cpp, not here.

+ /* 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<nsVideoDecoder> mDecoder;

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> mPresentationThread;

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(PR_MillisecondsToInterval(1000));

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_cast<short>(scaled_value);

Use "short(scaled_value)".

+ mLock(0),

nsnull

+ return mLock ? PR_TRUE : PR_FALSE;

mLock != nsnull

+ while (CanDecode()) {
+ ...

Read more...

I think the idea is that <video> does not have fallback content like <object>.

In , Dao (dao) wrote :

Yes. From <http://www.whatwg.org/specs/web-apps/current-work/multipage/section-video.html>:

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

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

>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(scaled_value)".

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 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(scaled_value)".
>
> 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.

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.

reinterpret_cast<> should give you anything you would need from a C-style cast, should it not?

Are there any plans to support the attributes specified by the spec? From my understanding this should be able to work:

<video src="test4_files/transformers480.ogg" controller="true" autoplay="true"/>

The patched version doesn't seem to support this yet?

The patch is not complete yet.

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.

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.

Re: comment 16, captioning-related URLs from maikmerten on #whatwg:
http://trac.annodex.net/wiki/CmmlSubtitles
http://wiki.xiph.org/index.php/Ogg_Skeleton
http://svn.xiph.org/trunk/writ/

(Annodex and Writ are different.)

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.

Don't forget to apply the third party modules patch first:

http://www.double.co.nz/video_test/third_party_modules.patch.gz

Some example usage of the video element here:

http://www.double.co.nz/video_test

Download full text (5.8 KiB)

+ // Media loading flags. See:
+ // http://www.whatwg.org/specs/web-apps/current-work/#video)
+ PRUint16 mNetworkState;
+ PRUint16 mReadyState;

Can we have typedefs for PRUint16 defined for these?

+ PRBool mBegun;
+ PRBool mLoadedFirstFrame;
+ PRBool mLoadedEnoughToPlayThrough;
+ 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(SYDNEY_AUDIO_NO_POSITION)
+ 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<nsVideoInvalidateEvent> mInvalidateEvent;
+ nsCOMPtr<nsVideoDecodeEvent> mDecodeEvent;
+ nsCOMPtr<nsVideoPresentationEvent> mPresentationEvent;

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_destroy((sa_stream_t*)mAudioHandle);

static_cast (and elsewhere)

+ *aTime = ((double)PR_IntervalToMilliseconds(PR_IntervalNow()))/1000.0 - mPauseTime;
+ *aTime = static_cast<double>(((bytes + mPauseBytes) * 1000 / mRate / (sizeof(short) * mChannels))) / 1000.0;

Constructor-style-cast

+ nsChannelReader* me = (nsChannelReader*)handle;

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

Read more...

Created an attachment (id=283664)
Version 6 of the <video> element patch

No new functionality from version 5. Addresses the review comments.

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

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.

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-release-conflations as we use on the CVS trunk for each Mozilla milestone (which then moves to a CVS branch).

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

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 other words, yes, I'd be willing to review the contet/DOM parts. On my free time if I have to :)

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.

Micah Gersten (micahg) on 2009-08-13
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
Micah Gersten (micahg) on 2010-01-26
Changed in firefox-3.5 (Ubuntu):
importance: Undecided → Wishlist
status: New → Triaged
Changed in firefox:
importance: Unknown → Wishlist
402 comments hidden view all 482 comments

I think you're ready to land.

Great, waiting for someone with commit access to merge then \o/

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 reply to Michael Monreal [:monreal] from comment #275)
> As GSteamer 1.0 is just around the corner...
>
> http://gstreamer.freedesktop.org/wiki/ZeroPointEleven
> http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/random/porting-to-
> 0.11.txt

Hehe, yeah that's on the todo list already :)

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-central/inbound in lockdown mode currently, this will need approval before landing.

Ok, thanks. I just wanted to be clear which patch we intended to land.

Try run for 1e0e98d406ac is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1e0e98d406ac
Results (out of 230 total builds):
    success: 196
    warnings: 34
Builds (or logs if builds failed) available at:
http://<email address hidden>

This is NPOTB (not part of the default build) and should land.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=1e0e98d406ac is clean; it shows two Mochitest orange, both of which are known in, and a bunch of restarts after infrastructure or timeout issues.

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://wiki.mozilla.org/Tree_Rules#mozilla-central_.28Nightly_channel.29

Keywords field has "mobile" but platform is "all." So, is this still fennec only or should this work with Firefox, including on Windows?

Should work with Firefox on any platform, including Windows.

In , Dao (dao) wrote :

(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://bugzilla.mozilla.org/describekeywords.cgi
The "mobile" keyword means "important to mobile developmental work", not mobile-only.

Changed in firefox:
status: In Progress → Fix Released

Thank you for your excellent patch. Do you plan to add support of standalone mp3 audio (in the <audio> tag)?

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.

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

Could someone, please, compile a build for Windows with this special flag needed to include GStreamer support?

(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 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 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://ossbuild.googlecode.com (although it seems abandoned at v0.10.7). Will it work with GStreamer-enabled Nightly build?

(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://ossbuild.googlecode.com (although it seems abandoned at v0.10.7).
> 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://is.gd/tteunv.

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

The upstream fix is preffed off by default, can we get --enable-gstreamer in the ubuntu builds? (version 15+)

(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://gstreamer.com/

(In reply to Sid from comment #298)
> Just in case, SDK is out.
> http://gstreamer.com/

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://docs.gstreamer.com/display/GstSDK/Installing+on+Windows

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.

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://www.gnu.org/licenses/gpl-faq.en.html#WindowsRuntimeAndGPL).

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.

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 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://archive.msdn.microsoft.com/KB956414. But indeed it's very clear here that you are allowed to redistribute them even without the installer (http://msdn.microsoft.com/en-us/library/ms235299.aspx)

Can we introduce GStreamer to Mozilla source tree?

Because on Windows, Gstreamer need rebuilding using the user's compiler.

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

sam tygier (samtygier) on 2012-07-26
description: updated
Oibaf (oibaf) on 2012-09-07
description: updated
description: updated
description: updated
Oibaf (oibaf) on 2012-09-07
description: updated
Oibaf (oibaf) on 2012-09-07
description: updated
sam tygier (samtygier) wrote :

"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 :

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 :

having trouble with launchpad, but probably interesting to look at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=682917 and https://bugzilla.redhat.com/show_bug.cgi?id=843583

Are there plans to enable this in Linux builds? Should that be a separate Mozilla bug?

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 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://sourcecode.opera.com/gstreamer/

sam tygier (samtygier) wrote :

there is a more focused discussion on Bug #1051559

Displaying first 40 and last 40 comments. View all 482 comments or add a comment.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Related questions

Remote bug watches

Bug watches keep track of this bug in other bug trackers.