Closed Bug 806917 Opened 12 years ago Closed 10 years ago

support GStreamer 1.0

Categories

(Core :: Audio/Video, enhancement)

All
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- affected
relnote-firefox --- 30+

People

(Reporter: wolfiR, Assigned: alessandro.d)

References

Details

(Keywords: feature)

Attachments

(5 files, 17 obsolete files)

82.20 KB, image/svg+xml
Details
81.98 KB, patch
Details | Diff | Splinter Review
389 bytes, patch
gps
: review+
dholbert
: feedback+
Details | Diff | Splinter Review
99.89 KB, image/png
Details
470.74 KB, image/png
Details
Linux distributions coming up from now on do likely support GStreamer 1.0 only.
The implementation might need to be updated in Gecko.

Given that the Firefox process already maps gstreamer libraries (through libcanberra) it's not really an option to have 0.10 in parallel to 1.0 as there are symbol clashes between both AFAIK.

Porting information:
http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/random/porting-to-1.0.txt
Attached patch First pass at a patch. (obsolete) — Splinter Review
I have made a patch to optionally build against 1.0, but it may need work. I'm not sure how best to test it (I get a lot of unexpected mochitest failures and timeouts without my patch; I'm not sure if that's normal or if there is something strange about my set-up).
Great work Mike!

Alessandro Decina should be the one to review your patch, he's got the best understanding of the GStreamer backend.

I've made this bug block bug 833628 because I was tracking fixing a particular shutdown hang that I saw with the GStreamer backend in mochitests there.
Blocks: 833628
Blocks: 794282
Attached patch Work in progress. (obsolete) — Splinter Review
Work in progress to update to apply against the current code base; I'm trying to figure out how the fix for bug 761018 should be handled (ie, one should call gst_buffer_map for access to the data).

Also, do you have advice for testing this? Running the mochitests gives me a timeout if gstreamer is enabled regardless of whether I apply this patch (probably bug 833628).
Attachment #729607 - Flags: feedback?(alessandro.d)
Attached patch WIP 1.0 patch (obsolete) — Splinter Review
Working 1.0 support. This is still WIP, I need to find a way to reduce the #ifdefs and fix seeking (seek forward works, backwards is broken).
Attachment #714832 - Attachment is obsolete: true
Attachment #729607 - Attachment is obsolete: true
Attachment #729607 - Flags: feedback?(alessandro.d)
Attached patch WIP 1.0 support (obsolete) — Splinter Review
Seeking works now and I started moving the 0.10 specific code to GStreamerReader-0.10.cpp
Attachment #734530 - Attachment is obsolete: true
I'm getting a few link errors on linux with this new patch:
/usr/bin/ld.gold.real: error: hidden symbol 'gst_video_meta_api_get_type' is not defined locally
/usr/bin/ld.gold.real: error: hidden symbol 'gst_video_meta_map' is not defined locally
/usr/bin/ld.gold.real: error: hidden symbol 'gst_video_meta_unmap' is not defined locally
/usr/bin/ld.gold.real: error: hidden symbol 'gst_buffer_pool_config_get_video_alignment' is not defined locally
/usr/bin/ld.gold.real: error: hidden symbol 'gst_buffer_pool_config_get_video_alignment' is not defined locally
/usr/bin/ld.gold.real: error: hidden symbol 'gst_video_meta_api_get_type' is not defined locally
/usr/bin/ld.gold.real: error: hidden symbol 'gst_video_buffer_pool_get_type' is not defined locally

All of these functions appear to be defined in libgstvideo, and libgstvideo is being linked properly.

There were also some messages about gst_memory_is_type not being defined (doesn't appear to be in 1.0), but got around that by using GST_IS_MOZ_GFX_MEMORY_ALLOCATOR instead.


Compiles fine on my Mac; audio works, but gstreamer fails to build the pipeline for video. I can get video to play from gst-launch with `filesrc ! qtdemux ! vtdec_h264 ! videoconvert ! autovideosink'. When trying to play through Firefox though, video goes through `qtdemux ! multiqueue ! h264parse' and stops there. Will attach pipeline dump.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> I'm getting a few link errors on linux with this new patch:

Actually, looks like I've just hosed my gstreamer setup somehow on my linux box. Grr.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
 
> Compiles fine on my Mac; audio works, but gstreamer fails to build the
> pipeline for video. I can get video to play from gst-launch with `filesrc !
> qtdemux ! vtdec_h264 ! videoconvert ! autovideosink'. When trying to play
> through Firefox though, video goes through `qtdemux ! multiqueue !
> h264parse' and stops there. Will attach pipeline dump.

Hm, have you tried to play from gst-launch with playbin?
Attached file 1.0 support (obsolete) —
New version of the patch. To remove some of the #if GST_VERSION_MAJOR clutter, I refactored the code slightly and split the largest 0.10 specific bits in GStreamerReader-0.10.cpp.

Playback with 1.0 (--enable-gstreamer=1.0) works but there's still some work left to do to pass the test suite. When used with gst 0.10 (--enable-gstreamer=0.10 or simply --enable-gstreamer) the patch introduces no regressions when running the test suite.
Attachment #746297 - Attachment is obsolete: true
Attached patch 1.0 support (obsolete) — Splinter Review
Same as the previous patch, with a fix in configure.in so that --enable-gstreamer=X actually works
Attachment #748471 - Attachment is obsolete: true
(In reply to Alessandro Decina from comment #9)
> Hm, have you tried to play from gst-launch with playbin?

Running:

gst-launch-1.0 playbin uri=file:///Users/edwin/bruce.mp4

I get the message:

WARNING: from element /GstPlayBin:playbin0/GstURIDecodeBin:uridecodebin0: No decoder available for type 'video/x-h264, stream-format=(string)avc, alignment=(string)au, level=(string)5, profile=(string)high, codec_data=(buffer)01640032ffe1001b67640032ac34e601e0089f961000000300100000030300f18319a001000668e9784cb22c, width=(int)1920, height=(int)1080, framerate=(fraction)24/1, pixel-aspect-ratio=(fraction)1/1, parsed=(boolean)true'.

The plugin I'm trying to get it to use is the applemedia vtdec_h264 plugin in gst-plugins-bad. The sink caps look like they should accept the above type:

    Capabilities:
      video/x-h264
                  width: [ 1, 2147483647 ]
                 height: [ 1, 2147483647 ]
              framerate: [ 0/1, 2147483647/1 ]
          stream-format: avc
Attached patch Small fixes (obsolete) — Splinter Review
Fixes for a few small issues I had on linux:
 - gst_memory_is_type doesn't exist in gstreamer <= 1.0
 - When upstream doesn't negotiate allocator, CopyIntoImageBuffer would fail because |mBufferPool| isn't configured
 - gst/video/gstvideo{meta,pool}.h needed to be included
 - Had to force visibility in above libs to "default" to avoid link errors
Attachment #748637 - Flags: feedback?(alessandro.d)
(In reply to Edwin Flores [:eflores] [:edwin] from comment #13)
> Created attachment 748637 [details] [diff] [review]
> Small fixes

Looks good, I'll integrate these fixes

 
> Fixes for a few small issues I had on linux:
>  - gst_memory_is_type doesn't exist in gstreamer <= 1.0

It was added in 1.2, what 1.x version do you have? Anyway the patch is simple enough so let's put it in for now.


>  - When upstream doesn't negotiate allocator, CopyIntoImageBuffer would fail
> because |mBufferPool| isn't configured

Good catch. I'm not 100% sure that passing GST_CAPS_ANY is safe there, i'll double check.


>  - gst/video/gstvideo{meta,pool}.h needed to be included
>  - Had to force visibility in above libs to "default" to avoid link errors

Hm, I guess these have to do with the fact that you're using an older 1.x than me
I'm using 1.0.5. I think most distros are on 1.0.x, so it's the most desirable target for now.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #15)
> I'm using 1.0.5. I think most distros are on 1.0.x, so it's the most
> desirable target for now.

0.10 is still the default that comes on a lot of systems. Some systems don't even have gstreamer 1.0 (like current Ubuntu LTS). For additional fun, if some system libraries are using gstreamer 0.10, using gstreamer 1.0 is a pile of bugs waiting to happen.
> 0.10 is still the default that comes on a lot of systems. Some systems don't
> even have gstreamer 1.0 (like current Ubuntu LTS). For additional fun, if
> some system libraries are using gstreamer 0.10, using gstreamer 1.0 is a
> pile of bugs waiting to happen.

gstreamer 0.10 is EOF upstream so 1.0 is highly desirable and has much better Mac/Windows support. Even with that they are both parallel installable without too many problems on Linux.
(In reply to pbrobinson from comment #17)
> > 0.10 is still the default that comes on a lot of systems. Some systems don't
> > even have gstreamer 1.0 (like current Ubuntu LTS). For additional fun, if
> > some system libraries are using gstreamer 0.10, using gstreamer 1.0 is a
> > pile of bugs waiting to happen.
> 
> gstreamer 0.10 is EOF upstream

Upstream doesn't choose what is on actual user systems, unfortunately.

> so 1.0 is highly desirable and has much
> better Mac/Windows support. Even with that they are both parallel
> installable without too many problems on Linux.

They are parallel installable, but not parallel linkable. So if the user system has, say, libcanberra-gstreamer installed, which is using gstreamer 0.10 on that system, and which we load for event sounds (and that gtk uses too, iirc), and we load gstreamer 1.0, "fun" bugs will show up.
(In reply to Mike Hommey [:glandium] from comment #18)
> (and that gtk uses too, iirc)

That would be libgnome.
(That's why bug 859199 is essential).
(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> Compiles fine on my Mac; audio works, but gstreamer fails to build the
> pipeline for video. I can get video to play from gst-launch with `filesrc !
> qtdemux ! vtdec_h264 ! videoconvert ! autovideosink'. When trying to play
> through Firefox though, video goes through `qtdemux ! multiqueue !
> h264parse' and stops there. Will attach pipeline dump.

Found it! vtdec_h264 has GST_RANK_NONE, but playbin filters down to only GST_RANK_MARGINAL.

This should be fine, though. If we're include gstreamer in the build, we can just patch over it to set the rank higher.
Depends on: 878363
What's the progress on this now?
I'm Interested in firefox playing h264 video under raspberry Pi.

Altought current version of Debian/Raspian linux use Gtreamer 0.10,  porting of gstreamer 1.0 is already working (version 1.08 ad apt package)
Gstreamer 1.0 allow use of hardware decoding of h264, but 

I kindly ask if your patch could be applied to modify firefox/iceweasel 10.0.12 in Raspberry, so to achieve hardware video decoding.

Could it be possible ?
Hi Giuliano. That's a question for the maintainers of your Raspberry Pi distribution. We don't support Firefox 10 any more; your best bet for this feature would be to wait until a more recent version is packaged for your distro.
thanks RAlph.
Currente updates in debian have realesed firefox/iceseasel 17
http://plugwash.raspbian.org/

should it be compatible for your patch ?
The big question is

which version of firefox is compatible with gstreamer 1.0 ?
(In reply to Giuliano Lotta from comment #26)
> which version of firefox is compatible with gstreamer 1.0 ?

None yet till the patch lands in mozilla source tree.
And the patch itself for which version (= or >= )is realized ?
Attached patch WIP (obsolete) — Splinter Review
Did a bit of hacking on this a couple weeks ago; thought I should put this up.

Fixed a couple seeking issues, and with that a few tests fixed themselves.

Also found that we'd get bogus metadata using anything other than the GST_VIDEO_FRAME_* macros on 1.0.

When upstream fails to negotiate the allocator, we used to instantiate one and then copy the buffer over. Now we just copy to a PlanarYCbCrImage directly.

Outstanding issues:
 * Some tests still fail.
 * gst_buffer_pool_config_get_video_alignment returns total rubbish, even when the option GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT is present. For now, gst_video_info_align is commented out.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #29)
> Outstanding issues:
There is also still a shutdown hang sometimes.
This's going to help with va-api also.
1.2 has been released. Should this be the target instead? Seems to be mostly compatible with 1.0.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #29)
>  * gst_buffer_pool_config_get_video_alignment returns total rubbish, even
> when the option GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT is present. For now,
> gst_video_info_align is commented out.
I'm no expert but shouldn't you check that return value of gst_buffer_pool_config_get_video_alignment is TRUE before using what it produces? Based on quick reading of gst looks like if it doesn't return TRUE, it failed to parse config.
(In reply to seppo.yli-olli from comment #33)
> (In reply to Edwin Flores [:eflores] [:edwin] from comment #29)
> >  * gst_buffer_pool_config_get_video_alignment returns total rubbish, even
> > when the option GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT is present. For now,
> > gst_video_info_align is commented out.
> I'm no expert but shouldn't you check that return value of
> gst_buffer_pool_config_get_video_alignment is TRUE before using what it
> produces? Based on quick reading of gst looks like if it doesn't return
> TRUE, it failed to parse config.

Yeah. Last weekend I rebased the patch and fixed this and a few more issues (OMTC broke the patch apparently). I will post an update soonish.
Updated, rebased patch.

Edwin: I couldn't rebase your last patch (https://bugzilla.mozilla.org/attachment.cgi?id=788730), as I think it's applied on some other unpublished branch. I tried to get all the changes in and I *hope* I didn't miss anything.
Attachment #788730 - Attachment is obsolete: true
Depends on: 928806
Comment on attachment 819584 [details] [diff] [review]
0001-Bug-806917-support-GStreamer-1.0.patch

Setting r? so I remember to review/test this patch soon. If it doesn't regress 0.10 and code review is okay then there's no reason we can't land it.
Attachment #819584 - Flags: review?(edwin)
Hi, I was wandering - considering the fact that the already available GStreamer 1.2 plus the soon-to-be-released version of gstreamer-vaapi allow for hardware acceleration on Linux desktop - if that mean that when using GStreamer (for H.264 decoding) we will get hardware acceleration whereas when using internal Firefox codec (for Theora & VP8 decoding) we will get no hardware acceleration ? Thank you in advance for claryfying that point!
Hardware H.264 support through VA-API is possible but not yet supported. Hardware decoding of VP8 is not supported by VA-API.
Comment on attachment 819584 [details] [diff] [review]
0001-Bug-806917-support-GStreamer-1.0.patch

+ r? khuey to review the change to configure.in (and moz.build?).
Attachment #819584 - Flags: review?(khuey)
Attached patch 806917-nov-fixes.patch (obsolete) — Splinter Review
Needed at least these fixes to get it near working on linux.

Even after this patch, I don't get any video, audio is distorted, and for some reason the time bar only updates once every few seconds at the quickest. This happens on both 0.10 and 1.0.
Comment on attachment 819584 [details] [diff] [review]
0001-Bug-806917-support-GStreamer-1.0.patch

Putting off review until linux regressions can be dealt with.
Attachment #819584 - Flags: review?(khuey)
Attachment #819584 - Flags: review?(edwin)
Edwin, I filed two issues that were giving similar behaviour on Mac: https://bugzilla.mozilla.org/show_bug.cgi?id=928806 and https://bugzilla.mozilla.org/show_bug.cgi?id=928797
Blocks: 947287
Attached patch new patch (obsolete) — Splinter Review
New, feature complete patch!

I have tested this on Mac in the following configurations:

* --enable-gstreamer=1.0, with system VideoToolbox and AudioToolbox based decoders

* --enable-gstreamer=0.10, with gst-libav decoders

Both pass the test suite (with one caveat: I had to TODO one test, which is TODO everywhere except win8).

I haven't tested this on linux yet. I am setting up a linux VM now but I would appreciate help testing on linux.
Attachment #748518 - Attachment is obsolete: true
Attachment #819584 - Attachment is obsolete: true
Attachment #830538 - Attachment is obsolete: true
Attachment #8347734 - Flags: review?(edwin)
And just to make sure people don't miss it, you need to apply the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=928806 for this to work. That patch is flagged checkin-needed so it will hopefully be merged soon.
Works fine here on Linux (Debian/unstable, GStreamer 1.2.1), using gst-libav for h264 decoding.

Also I don't see anything obviously wrong in the patch, the changes look good to me.
Comment on attachment 8347734 [details] [diff] [review]
new patch

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

::: content/media/test/manifest.js
@@ +359,5 @@
>  // Unfortunately big-buck-bunny-unseekable.mp4 is doesn't play on Windows 7, so
> +// only include it in the unseekable tests if we're on later versions of Windows. 
> +// The file doesn't play on Mac as well (gives an error as the index is
> +// corrupt).
> +if ((navigator.userAgent.indexOf("Windows") == -1 && navigator.userAgent.indexOf("Macintosh") == -1) ||

This will fail on Linux too with GStreamer 1.0, so you'll need to add that to this check.

FYI, big-buck-bunny-unseekable.mp4 is a regular MP$4 file with the first MDAT truncated by 158703 bytes, that is, roughly half of the first MDAT is missing.
With --enable-warnings-as-errors and clang:

20:16.73 /home/alex/mozilla-central/content/media/gstreamer/GStreamerReader.cpp:77:3: error: field 'mConfigureAlignment' will be initialized after field 'fpsNum' [-Werror,-Wreorder]
20:16.73   mConfigureAlignment(true),
20:16.73   ^
20:16.76 /home/alex/mozilla-central/content/media/gstreamer/GStreamerReader.cpp:1237:17: error: unused variable 'info' [-Werror,-Wunused-variable]
20:16.76   GstVideoInfo *info = &mVideoInfo;
20:16.76                 ^
20:16.93 2 errors generated.
20:16.94 In the directory  /home/alex/mozilla-central/obj-x86_64-unknown-linux-gnu/content/media/gstreamer
20:16.94 The following command failed to execute properly:
20:16.94 /usr/bin/ccache clang++ -o GStreamerReader.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /home/alex/mozilla-central/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/home/alex/mozilla-central/content/media/gstreamer -I. -I/home/alex/mozilla-central/content/base/src -I/home/alex/mozilla-central/content/html/content/src -I../../../dist/include -I/home/alex/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/include/nspr -I/home/alex/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/include/nss -fPIC -Qunused-arguments -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MP -MF .deps/GStreamerReader.o.pp -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -Wno-error=uninitialized -Wno-error=deprecated-declarations -march=native -O2 -pipe -march=native -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -I/home/alex/mozilla-central/build/unix/headers -pthread -pipe -DNDEBUG -DTRIMMED -g -Xclang -load -Xclang ../../../build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fomit-frame-pointer -Werror -pthread -I/usr/include/gstreamer-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include /home/alex/mozilla-central/content/media/gstreamer/GStreamerReader.cpp
20:16.94 gmake[5]: *** [GStreamerReader.o] Error 1
(In reply to Chris Pearce (:cpearce) from comment #52)
> Comment on attachment 8347734 [details] [diff] [review]
> new patch
> 
> Review of attachment 8347734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/manifest.js
> @@ +359,5 @@
> >  // Unfortunately big-buck-bunny-unseekable.mp4 is doesn't play on Windows 7, so
> > +// only include it in the unseekable tests if we're on later versions of Windows. 
> > +// The file doesn't play on Mac as well (gives an error as the index is
> > +// corrupt).
> > +if ((navigator.userAgent.indexOf("Windows") == -1 && navigator.userAgent.indexOf("Macintosh") == -1) ||
> 
> This will fail on Linux too with GStreamer 1.0, so you'll need to add that
> to this check.

Ah yes, thanks.
(In reply to Alex Xu from comment #53)
> With --enable-warnings-as-errors and clang:
[snip]

Fixed both warnings locally. Thanks!
Comment on attachment 8347734 [details] [diff] [review]
new patch

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

Looks good so far! I still haven't had the chance to review this properly, but I tested it briefly yesterday.

It looks like it doesn't regress 0.10 too much, so I would be comfortable landing this with 0.10 remaining the default after the tests are sorted out: it sounds like you know about test_unseekable, and on my machine test_reset_src also hangs.

1.0 still isn't working very well for me:
 * A lot of tests hanging. This could just be an event somewhere that isn't being fired.
 * Image format issues (in particular, with stride and colour format) when upstream doesn't negotiate the allocator. I'm fairly certain I fixed this in one of the obsolete patches.

Further, it doesn't look like gstreamer support is being built by default (I have to specify --enable-gstreamer in .mozconfig). At a glance, I can't see anything in configure.in that would cause that. Probably just need to look closer.

I'll be reviewing the code itself soon.

::: configure.in
@@ +5569,2 @@
>    MOZ_GSTREAMER=1
> +  GST_API_VERSION=1.0

0.10 for now.

@@ +5574,5 @@
> +[  --enable-gstreamer[=1.0]           Enable GStreamer support],
> +[ MOZ_GSTREAMER=1
> +  # API version, eg 0.10, 1.0 etc
> +  if test -z "$enableval" -o "$enableval" = "yes"; then
> +    GST_API_VERSION=1.0

Same here.
Attached patch bug806917.patch (obsolete) — Splinter Review
Attachment #8347734 - Attachment is obsolete: true
Attachment #8347734 - Flags: review?(edwin)
Attachment #8349295 - Flags: review?(edwin)
(In reply to Edwin Flores [:eflores] [:edwin] from comment #56)
> Comment on attachment 8347734 [details] [diff] [review]
> new patch
> 
> Review of attachment 8347734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good so far! I still haven't had the chance to review this properly,
> but I tested it briefly yesterday.

Thanks! :)


> 
> It looks like it doesn't regress 0.10 too much, so I would be comfortable
> landing this with 0.10 remaining the default after the tests are sorted out:
> it sounds like you know about test_unseekable, and on my machine
> test_reset_src also hangs.

The last patch skips that file with a broken mdat so test_unseekable should pass.
test_reset_src doesn't seem to fail for me, i've tried to run it with --run-until-failure for about 1h and it never failed. Could be due to some specific plugin/element version. I think i'll need help with that one :/


> 
> 1.0 still isn't working very well for me:
>  * A lot of tests hanging. This could just be an event somewhere that isn't
> being fired.
>  * Image format issues (in particular, with stride and colour format) when
> upstream doesn't negotiate the allocator. I'm fairly certain I fixed this in
> one of the obsolete patches.

Weird. I am running with the new vtdec in gstreamer git which doesn' negotiate the allocator with downstream. Maybe that's the issue actually, maybe _negotiating_ causes issues. I'll check that.


> 
> Further, it doesn't look like gstreamer support is being built by default (I
> have to specify --enable-gstreamer in .mozconfig). At a glance, I can't see
> anything in configure.in that would cause that. Probably just need to look
> closer.

I fixed this in the last patch.


> 
> I'll be reviewing the code itself soon.

Cool :)


> 
> ::: configure.in
> @@ +5569,2 @@
> >    MOZ_GSTREAMER=1
> > +  GST_API_VERSION=1.0
> 
> 0.10 for now.
> 
> @@ +5574,5 @@
> > +[  --enable-gstreamer[=1.0]           Enable GStreamer support],
> > +[ MOZ_GSTREAMER=1
> > +  # API version, eg 0.10, 1.0 etc
> > +  if test -z "$enableval" -o "$enableval" = "yes"; then
> > +    GST_API_VERSION=1.0
> 
> Same here.

Done
Depends on: 952769
Edwin, I think I managed to reproduce the stride issue you're seeing. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=952769 which fixes the underlying issue, and i'm going to refresh the gst patch soonish.
Attached patch new version (obsolete) — Splinter Review
Ok, this version + https://bugzilla.mozilla.org/show_bug.cgi?id=952769 should fix the stride issue with 1.0 and accelerated compositing
Attachment #8349295 - Attachment is obsolete: true
Attachment #8349295 - Flags: review?(edwin)
Attachment #8351261 - Flags: review?(edwin)
Comment on attachment 8351261 [details] [diff] [review]
new version

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

warning: squelched 12 whitespace errors
warning: 17 lines add whitespace errors.
On https://github.com/mozilla/gecko-dev/commit/d0a9cd5005025b6f1dd9a37431b67dcd109e79c1:

17:26.80 /home/alex/gecko-dev/content/media/gstreamer/GStreamerReader-0.10.cpp:124:17: error: no viable overloaded '='
17:26.80   data.mPicSize = gfxIntSize(mPicture.width, mPicture.height);
17:26.80   ~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17:26.81 ../../../dist/include/mozilla/gfx/Point.h:96:8: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'gfxIntSize' (aka 'nsIntSize') to 'const mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>' for 1st argument
17:26.81 struct IntSizeTyped :
17:26.82        ^
17:26.82 ../../../dist/include/mozilla/gfx/Point.h:96:8: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'gfxIntSize' (aka 'nsIntSize') to 'mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>' for 1st argument
17:26.82 struct IntSizeTyped :
17:26.82        ^
17:26.82 /home/alex/gecko-dev/content/media/gstreamer/GStreamerReader-0.10.cpp:129:15: error: no viable overloaded '='
17:26.82   data.mYSize = nsIntSize(data.mYStride,
17:26.82   ~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~
17:26.82 ../../../dist/include/mozilla/gfx/Point.h:96:8: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'nsIntSize' to 'const mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>' for 1st argument
17:26.82 struct IntSizeTyped :
17:26.82        ^
17:26.83 ../../../dist/include/mozilla/gfx/Point.h:96:8: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'nsIntSize' to 'mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>' for 1st argument
17:26.83 struct IntSizeTyped :
17:26.83        ^
17:26.83 /home/alex/gecko-dev/content/media/gstreamer/GStreamerReader-0.10.cpp:133:18: error: no viable overloaded '='
17:26.83   data.mCbCrSize = nsIntSize(data.mCbCrStride,
17:26.83   ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
17:26.83 ../../../dist/include/mozilla/gfx/Point.h:96:8: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'nsIntSize' to 'const mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>' for 1st argument
17:26.83 struct IntSizeTyped :
17:26.83        ^
17:26.83 ../../../dist/include/mozilla/gfx/Point.h:96:8: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'nsIntSize' to 'mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>' for 1st argument
17:26.83 struct IntSizeTyped :
17:26.83        ^
17:27.14 3 errors generated.
17:27.16 In the directory  /home/alex/gecko-dev/obj-x86_64-unknown-linux-gnu/content/media/gstreamer
17:27.16 The following command failed to execute properly:
17:27.16 /usr/bin/ccache clang++ -o GStreamerReader-0.10.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /home/alex/gecko-dev/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/home/alex/gecko-dev/content/media/gstreamer -I. -I/home/alex/gecko-dev/content/base/src -I/home/alex/gecko-dev/content/html/content/src -I../../../dist/include -I/home/alex/gecko-dev/obj-x86_64-unknown-linux-gnu/dist/include/nspr -I/home/alex/gecko-dev/obj-x86_64-unknown-linux-gnu/dist/include/nss -fPIC -Qunused-arguments -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MP -MF .deps/GStreamerReader-0.10.o.pp -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -Wno-error=uninitialized -Wno-error=deprecated-declarations -O2 -pipe -march=native -march=native -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -I/home/alex/gecko-dev/build/unix/headers -pthread -pipe -DNDEBUG -DTRIMMED -g -Xclang -load -Xclang ../../../build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fomit-frame-pointer -Werror -pthread -I/usr/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/libxml2 /home/alex/gecko-dev/content/media/gstreamer/GStreamerReader-0.10.cpp
17:27.16 gmake[5]: *** [GStreamerReader-0.10.o] Error 1
Attachment #8351261 - Attachment is obsolete: true
Attachment #8351261 - Flags: review?(edwin)
Attachment #8355558 - Flags: review?(edwin)
Looks great! Works for me apart from that test_reset_src timeout, but it's okay on try so the problem could just be gremlins specific to my setup.

I'll start reviewing soon so we can get this landed!
Comment on attachment 8355558 [details] [diff] [review]
rebased version after gfx::IntSize changes in m-c

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

Just a few little things and we should be good -- mostly style stuff.

Adding r? khuey to review the configure.in changes.

::: content/media/gstreamer/GStreamerAllocator.cpp
@@ +69,5 @@
> +  gsize maxsize = aSize + aParams->prefix + aParams->padding;
> +  gst_memory_init (GST_MEMORY_CAST (mem),
> +      (GstMemoryFlags)aParams->flags,
> +      aAllocator, NULL, maxsize, aParams->align,
> +      aParams->prefix, aSize);

New lines in parentheses should be aligned with the inside of the parenthesis.

@@ +154,5 @@
> +  /* fallback copy and is_span */
> +}
> +
> +void
> +moz_gfx_memory_allocator_set_reader(GstAllocator *aAllocator, GStreamerReader* aReader)

Pointer * placement should be consistent.

@@ +172,5 @@
> +void
> +moz_gfx_buffer_pool_reset_buffer (GstBufferPool* aPool, GstBuffer* aBuffer)
> +{
> +  GstMemory* mem = gst_buffer_peek_memory(aBuffer, 0);
> +  

Trailing whitespace here and throughout the patch.

::: content/media/gstreamer/GStreamerReader-0.10.cpp
@@ +40,5 @@
> +                                                       GstBuffer** aBuf,
> +                                                       nsRefPtr<PlanarYCbCrImage>& aImage)
> +{
> +  /* allocate an image using the container */
> +  ImageContainer* container = mDecoder->GetImageContainer();

Should null check this. If, for example, we're in an <audio> element, GetImageContainer() will return null. If somebody puts a video in an <audio> element we'll segfault here.

@@ +49,5 @@
> +  /* prepare a GstBuffer pointing to the underlying PlanarYCbCrImage buffer */
> +  GstBuffer* buf = GST_BUFFER(gst_moz_video_buffer_new());
> +  GST_BUFFER_SIZE(buf) = aSize;
> +  /* allocate the actual YUV buffer */
> +  GST_BUFFER_DATA(buf) = image->AllocateAndGetNewBuffer(aSize);

We should check that this allocation actually succeeded.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +36,3 @@
>  static const unsigned int MAX_CHANNELS = 4;
>  // Let the demuxer work in pull mode for short files
> +static const int SHORT_FILE_SIZE = 0;

Why are we taking this out?

@@ +126,3 @@
>  nsresult GStreamerReader::Init(MediaDecoderReader* aCloneDonor)
>  {
> +#if 0

Remove unused code.

@@ +154,5 @@
>    g_object_set(mPlayBin, "buffer-size", 0, nullptr);
>    mBus = gst_pipeline_get_bus(GST_PIPELINE(mPlayBin));
>  
>    mVideoSink = gst_parse_bin_from_description("capsfilter name=filter ! "
> +      "appsink name=videosink sync=false max-buffers=1 "

I've found that if video sync = audio sync = false, then gstreamer tries to sync video with audio, which sometimes conflicts with gecko's a/v sync.

I worked around this in bug 960599 by setting video sync = true, audio sync = false, and unsetting the pipeline clock.

@@ +431,5 @@
>    *aTags = nullptr;
>  
>    // Watch the pipeline for fatal errors
> +#if GST_VERSION_MAJOR >= 1
> +  gst_bus_set_sync_handler(mBus, GStreamerReader::ErrorCb, this, NULL);

nullptr, please.
Attachment #8355558 - Flags: review?(khuey)
Comment on attachment 8355558 [details] [diff] [review]
rebased version after gfx::IntSize changes in m-c

302 not me
Attachment #8355558 - Flags: review?(khuey) → review?(gps)
Attached patch 1.0.patch (obsolete) — Splinter Review
review fixes
Attachment #8355558 - Attachment is obsolete: true
Attachment #8355558 - Flags: review?(gps)
Attachment #8355558 - Flags: review?(edwin)
Attachment #8364193 - Flags: review?(edwin)
Attachment #8364193 - Flags: review?(gps)
(In reply to Edwin Flores [:eflores] [:edwin] from comment #65)
> Comment on attachment 8355558 [details] [diff] [review]
> rebased version after gfx::IntSize changes in m-c
> 
> Review of attachment 8355558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few little things and we should be good -- mostly style stuff.

Great, thanks for the review! I have attached a new version that should fix all the points you raised


> ::: content/media/gstreamer/GStreamerReader.cpp
> @@ +36,3 @@
> >  static const unsigned int MAX_CHANNELS = 4;
> >  // Let the demuxer work in pull mode for short files
> > +static const int SHORT_FILE_SIZE = 0;
> 
> Why are we taking this out?

I have added a comment in the source. Essentially this used to be an optimization to please mochitests that were checking ogg durations using obnoxiously high accuracy. Since we are not using gst for ogg, and since the optimization seems to make qtdemux a little slower (seeking too much), I disabled it. We can probably take it out entirely, but before doing so I want to check why qtdemux is seeking too much so I'd like to keep the code there although disabled for now.
(In reply to Alessandro Decina from comment #68)
> (In reply to Edwin Flores [:eflores] [:edwin] from comment #65)
> > Comment on attachment 8355558 [details] [diff] [review]
> > rebased version after gfx::IntSize changes in m-c
> > 
> > Review of attachment 8355558 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Just a few little things and we should be good -- mostly style stuff.
> 
> Great, thanks for the review! I have attached a new version that should fix
> all the points you raised
> 
> 
> > ::: content/media/gstreamer/GStreamerReader.cpp
> > @@ +36,3 @@
> > >  static const unsigned int MAX_CHANNELS = 4;
> > >  // Let the demuxer work in pull mode for short files
> > > +static const int SHORT_FILE_SIZE = 0;
> > 
> > Why are we taking this out?
> 
> I have added a comment in the source. Essentially this used to be an
> optimization to please mochitests that were checking ogg durations using
> obnoxiously high accuracy. Since we are not using gst for ogg, and since the
> optimization seems to make qtdemux a little slower (seeking too much), I
> disabled it. We can probably take it out entirely, but before doing so I
> want to check why qtdemux is seeking too much so I'd like to keep the code
> there although disabled for now.

Old code is for version control. If you think it's necessary, keep it in. If not, delete it. If you need it later, fetch it from history or this bug.
Attached patch 1.0.patch (obsolete) — Splinter Review
Fixed a few more nsIntSize => gfx::IntSize occurrencies

https://tbpl.mozilla.org/?tree=Try&rev=b1ab12fdce27
Attachment #8364193 - Attachment is obsolete: true
Attachment #8364193 - Flags: review?(gps)
Attachment #8364193 - Flags: review?(edwin)
Attachment #8364791 - Flags: review?(gps)
Attachment #8364791 - Flags: review?(edwin)
ping? :)
Attachment #748637 - Attachment is obsolete: true
Attachment #748637 - Flags: feedback?(alessandro.d)
Applying: Bug 806917 - support GStreamer 1.0
/home/alex/gecko-dev/.git/rebase-apply/patch:2377: trailing whitespace.
// only include it in the unseekable tests if we're on later versions of Windows. 
warning: 1 line adds whitespace errors.
Running this patch and watching YouTube videos appears to cause a memory leak. ~1.2GB RSS, about:memory shows 678.47 MB (67.23%) ── heap-unclassified.

gstreamer 1.0.10, gecko-dev:

commit 6d78047748cf188f7af6aa360d5f3a1b6101bec1
Merge: c41c7a4 162544b
Author: Carsten "Tomcat" Book <cbook@mozilla.com>
Date:   2014-01-30 10:59:15 +0100

    merge b2g-inbound to mozilla-central

.mozconfig:

export CC="clang"
export CXX="clang++"
ac_add_options --disable-accessibility
ac_add_options --disable-crashreporter
ac_add_options --disable-dbm
ac_add_options --disable-dbus
ac_add_options --disable-gamepad
ac_add_options --disable-gconf
ac_add_options --disable-gio
ac_add_options --disable-install-strip
ac_add_options --disable-necko-wifi
ac_add_options --disable-parental-controls
ac_add_options --disable-pulseaudio
ac_add_options --disable-safe-browsing
ac_add_options --disable-tests
ac_add_options --disable-updater
ac_add_options --disable-url-classifier
ac_add_options --disable-websms-backend
ac_add_options --enable-application=browser
ac_add_options --enable-clang-plugin
ac_add_options --enable-debug-symbols
ac_add_options --enable-gstreamer=1.0
ac_add_options --enable-media-navigator
ac_add_options --enable-official-branding
ac_add_options --enable-optimize=-O2
ac_add_options --enable-system-ffi
ac_add_options --enable-system-hunspell
ac_add_options --enable-system-sqlite
ac_add_options --enable-warnings-as-errors
ac_add_options --with-arch=native
ac_add_options --with-ccache
ac_add_options --with-pthreads
ac_add_options --with-system-cairo
ac_add_options --with-system-icu
ac_add_options --with-system-libvpx
ac_add_options --with-system-pixman
Comment on attachment 8364791 [details] [diff] [review]
1.0.patch

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

Build bits look fine.

::: configure.in
@@ +5649,2 @@
>      else
> +        AC_MSG_ERROR([gstreamer-plugins-base found, but no libgstvideo. Something has gone terribly wrong. Try reinstalling gstreamer-plugins-base; failing that, disable the gstreamer backend with --disable-gstreamer.])

did you mean "not" instead of "no"?
Attachment #8364791 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #75)
> Comment on attachment 8364791 [details] [diff] [review]
> 1.0.patch
> 
> Review of attachment 8364791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Build bits look fine.
> 
> ::: configure.in
> @@ +5649,2 @@
> >      else
> > +        AC_MSG_ERROR([gstreamer-plugins-base found, but no libgstvideo. Something has gone terribly wrong. Try reinstalling gstreamer-plugins-base; failing that, disable the gstreamer backend with --disable-gstreamer.])
> 
> did you mean "not" instead of "no"?

Native English speaker, both are acceptable. "not" may be slightly clearer, but some would go the other way.
(In reply to Alex Xu from comment #74)
> Running this patch and watching YouTube videos appears to cause a memory
> leak. ~1.2GB RSS, about:memory shows 678.47 MB (67.23%) ── heap-unclassified.

Can you link the video that causes the leak? Does it happen with one specific video or with any?

I can't reproduce the leak with gstreamer 1.2.2 (on mac, using both the hardware decoder and libav).
(In reply to Alessandro Decina from comment #77)
> (In reply to Alex Xu from comment #74)
> > Running this patch and watching YouTube videos appears to cause a memory
> > leak. ~1.2GB RSS, about:memory shows 678.47 MB (67.23%) ── heap-unclassified.
> 
> Can you link the video that causes the leak? Does it happen with one
> specific video or with any?
> 
> I can't reproduce the leak with gstreamer 1.2.2 (on mac, using both the
> hardware decoder and libav).

Any video, but upon further examination, it seems that my laptop on Firefox 26.0 is also experiencing large amounts of heap-unclassified memory usage.

This was after much more video watching, so it's possible that 1.0 exacerbates some existing issue.

For now, I suspect something wrong with my setup, but I will investigate more.
(In reply to Alex Xu from comment #78)
> (In reply to Alessandro Decina from comment #77)
> > (In reply to Alex Xu from comment #74)
> > > Running this patch and watching YouTube videos appears to cause a memory
> > > leak. ~1.2GB RSS, about:memory shows 678.47 MB (67.23%) ── heap-unclassified.
> > 
> > Can you link the video that causes the leak? Does it happen with one
> > specific video or with any?
> > 
> > I can't reproduce the leak with gstreamer 1.2.2 (on mac, using both the
> > hardware decoder and libav).
> 
> Any video, but upon further examination, it seems that my laptop on Firefox
> 26.0 is also experiencing large amounts of heap-unclassified memory usage.
> 
> This was after much more video watching, so it's possible that 1.0
> exacerbates some existing issue.
> 
> For now, I suspect something wrong with my setup, but I will investigate
> more.

Probably a behaviour change in libav: https://bugzilla.gnome.org/show_bug.cgi?id=721077
(In reply to Bastien Nocera from comment #79)
> (In reply to Alex Xu from comment #78)
> > (In reply to Alessandro Decina from comment #77)
> > > (In reply to Alex Xu from comment #74)
> > > > Running this patch and watching YouTube videos appears to cause a memory
> > > > leak. ~1.2GB RSS, about:memory shows 678.47 MB (67.23%) ── heap-unclassified.
> > > 
> > > Can you link the video that causes the leak? Does it happen with one
> > > specific video or with any?
> > > 
> > > I can't reproduce the leak with gstreamer 1.2.2 (on mac, using both the
> > > hardware decoder and libav).
> > 
> > Any video, but upon further examination, it seems that my laptop on Firefox
> > 26.0 is also experiencing large amounts of heap-unclassified memory usage.
> > 
> > This was after much more video watching, so it's possible that 1.0
> > exacerbates some existing issue.
> > 
> > For now, I suspect something wrong with my setup, but I will investigate
> > more.
> 
> Probably a behaviour change in libav:
> https://bugzilla.gnome.org/show_bug.cgi?id=721077

I do not use libav on my machines.

regardless, patch no longer applies.

Applying: Bug 806917 - support GStreamer 1.0
/home/alex/gecko-dev/.git/rebase-apply/patch:2377: trailing whitespace.
// only include it in the unseekable tests if we're on later versions of Windows. 
error: patch failed: content/media/gstreamer/GStreamerReader.cpp:861
error: content/media/gstreamer/GStreamerReader.cpp: patch does not apply
Patch failed at 0001 Bug 806917 - support GStreamer 1.0
The copy of the patch that failed is found in:
   /home/alex/gecko-dev/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Attachment #8364791 - Attachment is obsolete: true
59:57.95 /home/alex/gecko-dev/content/media/gstreamer/GStreamerReader.cpp:36:27: error: unused variable 'MAX_CHANNELS' [-Werror,-Wunused-const-variable]
59:57.96 static const unsigned int MAX_CHANNELS = 4;
59:57.96                           ^

clang version 3.4 (tags/RELEASE_34/final)
Target: x86_64-pc-linux-gnu
Thread model: posix

the variable is actually used, but as:

  NS_ASSERTION(mInfo.mAudio.mChannels > 0 && mInfo.mAudio.mChannels <= MAX_CHANNELS,
      "invalid audio channels number");

which means that it is unused in opt builds. (?)
Flags: needinfo?(alessandro.d)
Actually, I think this is in mozilla-central, not your patch.
Comment on attachment 8372598 [details] [diff] [review]
rebased

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

::: content/media/gstreamer/GStreamerFunctionList.h
@@ +127,5 @@
> +GST_FUNC(LIBGSTREAMER, gst_object_get_type)
> +GST_FUNC(LIBGSTREAMER, gst_pad_add_probe)
> +GST_FUNC(LIBGSTREAMER, gst_pad_get_current_caps)
> +GST_FUNC(LIBGSTREAMER, gst_pad_probe_info_get_query)
> +GST_FUNC(LIBGSTREAMER, gst_query_add_allocation_meta)

Do you still need the declaration after this version removed its usage in GStreamerReader::QueryProbe()?
Alex, this should fix the MAX_CHANNELS warning.

https://tbpl.mozilla.org/?tree=Try&rev=5e6f274b19df
Attachment #8372598 - Attachment is obsolete: true
Flags: needinfo?(alessandro.d)
(In reply to Alessandro Decina from comment #86)
> https://tbpl.mozilla.org/?tree=Try&rev=5e6f274b19df

That's not what I wanted to post...

...what I wanted to post is that i'm flagging this as checkin-needed as it was r+ and the leak seems to be in an older gst-libav version, not the patch itself. Also note that this isn't built by default yet, it has to be explicitly enabled by compiling with --enable-gstreamer=1.0.
Keywords: checkin-needed
I concur. If there aren't any regressions for supporting 0.10 then we're better to land it and fix any remaining issues as follow ups.
I just tested Alessandro's patch and it works.
\o/
This broke my local mozilla-inbound build, which had
 ac_add_options --disable-gstreamer
in the .mozconfig.

The build error looks like:
{
 0:02.43 configure:20638: checking for gstreamer-no >= 0.10.25
 0:02.43                       gstreamer-app-no
 0:02.43                       gstreamer-plugins-base-no
 0:02.43 configure: error: gstreamer and gstreamer-plugins-base development packages are needed to build gstreamer backend. Install them or disable gstreamer support with --disable-gstreamer
 0:02.43 *** Fix above errors and then restart with\
 0:02.43                "/usr/bin/make -f client.mk build"
}

Strangely, if I remove --disable-gstreamer from my .mozconfig, then everything works fine.

That seems fishy.
(I hit this build error on 64-bit Ubuntu 13.10, and I do have the gstreamer development packages installed, which makes this all the stranger. (I think I have the right ones installed, at least -- I have libgstreamer0.10-dev version 0.10.36-1.2ubuntu1 and libgstreamer-plugins-base0.10-dev version 0.10.36-1.1ubuntu1.))
gps, do you know what might be going on with Comment 91?
Flags: needinfo?(gps)
I can reproduce Comment 91 following the same steps as Daniel points out.
Here's a quick, untested fix to handle --disable-gstreamer properly.
Comment on attachment 8374293 [details] [diff] [review]
disable-gstreamer.patch

I can confirm that this fixes the issue for me.

AFAICT --disable-gstreamer is equivalent to --enable-gstreamer=no (or something like that), so without this patch, we treat --disable-gstreamer as "enable gstreamer, and require a GST_API_VERSION of "no", which means our PKG_CHECK_MODULES command looks for packages with a version >= "no", and that check fails.

Jan, can you request review (probably from gps) on this?
Attachment #8374293 - Flags: feedback+
(This explains the "no" suffix in "gstreamer-app-no" and "gstreamer-plugins-base-no" from comment 91)
Attachment #8374293 - Flags: review?(gps)
Flags: needinfo?(gps)
https://hg.mozilla.org/mozilla-central/rev/63cdfb958a52
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Just for clarification, does this mean that nightly builds will now support the GStreamer backend, and whatever codecs it supports?
That's been true for some time with the Linux builds. This bug in particular is about supporting the 1.0 gstreamer as well as 0.10.
Yes, as Ralph said. Switching to GST 1.0 because some modules already pull this version (e.g. libcanberra) and you can't link the two different versions.
In addition, the GStreamer backend only has a small whitelist of containers and codecs it is allowed to process, which contains mostly MPEG stuff (new and old).

Check the static arrays in content/media/gstreamer/GStreamerFormatHelper.cpp if you're interested.
Isn't that file converting the mime type format into gst_caps that GStreamer can understand? To help with the autoplugging which based on the caps selects which plugins to use for decoding.

Though I see your point, if the format isn't found it will not create a pipeline for it.
(In reply to Luis de Bethencourt [:luisbg] from comment #103)
> Isn't that file converting the mime type format into gst_caps that GStreamer
> can understand? To help with the autoplugging which based on the caps
> selects which plugins to use for decoding.
> 
> Though I see your point, if the format isn't found it will not create a
> pipeline for it.

No, the autoplugging is done by GStreamer itself using the typefind functionality. The only element the code makes is the playbin pipeline.

The GStreamerFormatHelper can check if GStreamer has the needed elements to decode a mime type (CanHandleMediaType). This is used to skip the GStreamer decoder if it would be useless anyway.

It also checks whether a set of caps is in part of the whitelist (CanHandleContainerCaps, CanHandleCodecCaps).
After prerolling the pipeline GStreamerReader makes sure the demuxers and decoders that GStreamer spawned only accept whitelisted caps.
That is what I meant, I agree with you. Maybe I miscommunicated :)
Blocks: 971609
Comment on attachment 8374293 [details] [diff] [review]
disable-gstreamer.patch

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

Thanks Jan! I don't have review powers but looks good to me!
I'm running the latest nightly, with the appropriate GStreamer plugins installed. But I don't see rendered video at this quick mockup I did:

http://www.perturb.org/tmp/test.html

"No video with supported mime type found". I'm running Nightly on Fedora 20 x86_64.
Comment on attachment 8374293 [details] [diff] [review]
disable-gstreamer.patch

[Tagging glandium for review on the followup, in case he gets to it before gps. I'm hoping we can land it ASAP, to avoid more pain from broken builds for other folks with --disable-gstreamer who have yet to pull in the main patch here.]
Attachment #8374293 - Flags: review?(mh+mozilla)
Comment on attachment 8374293 [details] [diff] [review]
disable-gstreamer.patch

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

Oy. Configure can be fickle.
Attachment #8374293 - Flags: review?(mh+mozilla)
Attachment #8374293 - Flags: review?(gps)
Attachment #8374293 - Flags: review+
Landed the followup (attributing it to Jan), to fix --disable-gstreamer builds:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/02f0327bf8e1

Thanks for the patch, Jan!
Kudos to Alessandro for all this cool work :)
(In reply to Scott Baker from comment #107)
> I'm running the latest nightly, with the appropriate GStreamer plugins
> installed. But I don't see rendered video at this quick mockup I did:
> 
> http://www.perturb.org/tmp/test.html
> 
> "No video with supported mime type found". I'm running Nightly on Fedora 20
> x86_64.

That probably means that you do not have the correct plugins installed. You need gst-plugins-good and gst-libav.

Anyway, I think it's time we implement something to troubleshoot/avoid the case in which people are missing plugins at runtime. GStreamer has support for detecting missing plugins and notify the calling application. The application can react to the notification by showing an error and/or installing the missing plugins in order for decoding to continue.

For example if you watch something with the GNOME video player (or even the Epiphany web browser) under Fedora/Ubuntu and you're missing some plugins, you get a popup dialog asking you to install some packages. After the installation is finished decoding continues and the video starts playing. This is implemented through an external helper utility that distros configure through the GST_INSTALL_PLUGINS_HELPER environment variable. 

Implementing this behaviour is a no-brainer, since we can reuse the helper utility and it just becomes matter of adding ~10 lines of code in GStreamerReader. This leaves the question open as to what to do when GST_INSTALL_PLUGINS_HELPER is not set though. 

Ideas?
Alessandro,

That is a good idea. I'm going to create a new bug for it and assign it to myself.

Sounds good?
(In reply to Luis de Bethencourt [:luisbg] from comment #116)
> Alessandro,
> 
> That is a good idea. I'm going to create a new bug for it and assign it to
> myself.
> 
> Sounds good?

I created https://bugzilla.mozilla.org/show_bug.cgi?id=972638. Feel free to assign it to yourself \o/
There are a backport to Firefox Beta 28 or Aurora 29 ??
Flags: needinfo?
Uplift is not likely to be accepted AFAIK; this is a major change which has fairly large risks and minor benefits.
Flags: needinfo?
(In reply to Alex Xu from comment #120)
> Uplift is not likely to be accepted AFAIK; this is a major change which has
> fairly large risks and minor benefits.

Understand, thanks you.
relnote-firefox: --- → ?
Keywords: feature, relnote
(In reply to Alessandro Decina from comment #115)
> (In reply to Scott Baker from comment #107)
> > I'm running the latest nightly, with the appropriate GStreamer plugins
> > installed. But I don't see rendered video at this quick mockup I did:
> > 
> > http://www.perturb.org/tmp/test.html
> > 
> > "No video with supported mime type found". I'm running Nightly on Fedora 20
> > x86_64.
> 
> That probably means that you do not have the correct plugins installed. You
> need gst-plugins-good and gst-libav.

FWIW I have gstreamer1-libav-1.2.1-1.fc20.x86_64 and gstreamer-plugins-good-0.10.31-10.fc20.x86_64 installed and just get a spinning "progress" indicator when I click the play button on the test case from comment 107.  See attached screenshot.
QA Contact: alexandra.lucinet
Added in the release note database for 30.
Has this issue been resolved in the latest build, or do I need to compile from source and append the --enable-gstreamer=1.0 flag?
It's checked in but gstreamer 0.10 is still the version used by default, so you need to configure with --enable-gstreamer=1.0 and compile if you want to use gstreamer 1.0.
Depends on: 989112
Are there any plans to --enable-gstreamer=1.0 in the default builds? gstreamer 0.10 is deprecated and fading out. On my Debian testing system, I already need to install several packages that were removed from the distribution months ago, to get the gstreamer 0.10 plugins that Firefox needs for video/mp4 playback. The same plugins are of course part of the distribution for gstreamer 1.0.
(In reply to Ralf Jung from comment #126)
> Are there any plans to --enable-gstreamer=1.0 in the default builds?
> gstreamer 0.10 is deprecated and fading out. On my Debian testing system, I
> already need to install several packages that were removed from the
> distribution months ago, to get the gstreamer 0.10 plugins that Firefox
> needs for video/mp4 playback. The same plugins are of course part of the
> distribution for gstreamer 1.0.

You should prob ask the debian packers that rather then here. Assuming you run firefox from the debian repo.
No, I run Firefox from the upstream website. Debian Firefox (Iceweasel) packages are often pretty old.
However, Debian is not the only distribution going that way (in terms of gstreamer) - OpenSUSE already had a release goal to get rid of the 0.10 version (they didn't achieve that goal, but I guess they will try again for the next release). Hence many users will still not have video/mp4 support as the gstreamer required to do that will not be installed, or the required plugins missing. gstreamer 0.10 has been deprecated more than a year ago, so distribution support will only shrink over time.
I second that, it would be great if Aurora and the following firefox builds would use GStreamer 1.x by default on Linux. All Linux distributions are upgrading from 0.10 and use 1.x by default these days.
+1 to changing the build defaults to GStreamer 1.x.
> 1.0 isn't available on Debian Stable.

I would argue that the default should be what Firefox prefers to use.  (This is probably the newer version).  It is trivial for others to add a flag and encourages them to use the better version.
The current thinking is to favour something that works everywhere. Now we have 1.0 support the distros can pick that up if they wish. That seems like a reasonable compromise right now.
Can't we support both? we're dlopening it, there's no reason we can't.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #133)
> The current thinking is to favour something that works everywhere.

Except that it doesn't: Current Debian testing does not have the ffmpeg plugin for gstreamer 0.10, so the Firefox gstreamer support is of no use there. The old plugin fails to build with current libav.
<http://packages.qa.debian.org/g/gstreamer0.10-ffmpeg.html>
The maintainer sees the libav plugin for gstreamer 1.0 as the replacement to use instead
<https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=720796#12>
So is this going to be configurable at runtime of it requires different builds for each variant? As others pointed out, Debian testing doesn't have gstreamer0.10-ffmpeg at all and only provides gstreamer1.0-libav. Debian stable can get gstreamer1.0-libav through Debian backports. So making gstreamer 0.10 the default beca
[sorry, the comment got submitted partially - contd.]:

So making gstreamer 0.10 the default in Firefox because of Debian doesn't make sense.
The issue is not the builds from the distro, but the official builds from Mozilla.
Just look at the current version in Debian testing - it's still at Firefox 24. So for anybody who wants to be up-to-date in terms of Firefox, the distro packages are not an option.
(In reply to Ralf Jung from comment #140)
> Just look at the current version in Debian testing - it's still at Firefox
> 24. So for anybody who wants to be up-to-date in terms of Firefox, the
> distro packages are not an option.

Unstable has 29, now, and experimental, 30.
How do I enable hardware acceleration with GST in FF?
Hardware acceleration is not supported.
I bet that it will, once GStreamer 1.0 is supported. You'll need GStreamer 1.2, gstreamer-vaapi 0.5.7 and the good hardware (see that list for Intel chips [1]). I can't see any reason why that would not be the case considering that GStreamer is able to do so...

[1] https://01.org/linuxgraphics/community/vaapi
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #144)
> Hardware acceleration is not supported.

Gstreamer support hardware acceleration even at 0.10.

It works on my system. Just need to sort the pipelines. FF needs to do that too.
Does not appear to work with (linux) FF 30.0. If I disable gstreamer0.10-ffmpeg the H.264 test video on this page does not work:
<http://www.quirksmode.org/html5/tests/video.html>
If I reenable gstreamer0.10-ffmpeg the video works properly. Tested with 32bit and 64bit.
The Configure arguments do not include an '--enable-gstreamer=1.0' statement:
--enable-update-channel=release --enable-update-packaging --with-google-api-keyfile=/builds/gapi.data --enable-crashreporter --enable-release --enable-elf-hack --enable-stdcxx-compat --enable-warnings-as-errors --enable-official-branding

Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:30.0) Gecko/20100101 Firefox/30.0

If I load up Ubuntu's 30.0, and run the same test by disabling gstreamer0.10-ffmpeg the video works. Pehaps the difference is that they do have '--enable-gstreamer=1.0':
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0
--host=x86_64-linux-gnu --prefix=/usr --libexecdir=/usr/lib/firefox --with-l10n-base=/build/buildd/firefox-30.0+build1/./l10n --srcdir=/build/buildd/firefox-30.0+build1/. --enable-release --disable-install-strip --disable-updater --enable-application=browser --enable-startup-notification --with-distribution-id=com.ubuntu --enable-optimize --enable-tests --enable-crashreporter --with-branding=browser/branding/official --disable-gnomevfs --enable-gio --enable-update-channel=release --disable-debug --disable-elf-hack --enable-gstreamer=1.0 --with-google-api-keyfile=/build/buildd/firefox-30.0+build1/debian/g
(In reply to NoOp from comment #147)
> Pehaps the difference is that they do
> have '--enable-gstreamer=1.0':

Yes, that's exactly the way to enable it at the moment (possible only at compile time).
Is it correct that this feature is not referring to audio but only to video? That's a huge disappointment, I was hoping for a better support of audio systems by this..
If yes, is there any plan to handle audio by gstreamer?
(In reply to nucrap from comment #149)
> Is it correct that this feature is not referring to audio but only to video?
> That's a huge disappointment, I was hoping for a better support of audio
> systems by this..
> If yes, is there any plan to handle audio by gstreamer?

It refers to audio as well. For example, with correct gstreamer plugins Firefox can play MP3 audio.
@Schmerl: Thanks, so how can I configure the audio output plugin for firefox? I need to use the oss4sink audio plugin. Btw I have seen another user on #gstreamer irc asking this very same question (but using jackd), so I guess many others would need this as well.
(In reply to nucrap from comment #151)
> @Schmerl: Thanks, so how can I configure the audio output plugin for
> firefox? I need to use the oss4sink audio plugin. Btw I have seen another
> user on #gstreamer irc asking this very same question (but using jackd), so
> I guess many others would need this as well.

Firefox uses GStreamer on linux only for decoding audio (and video), but then uses custom output code. It doesn't use GStreamer sinks.
(In reply to Alessandro Decina from comment #152)
> Firefox uses GStreamer on linux only for decoding audio (and video), but
> then uses custom output code. It doesn't use GStreamer sinks.

Are there any plans to use gstreamer sinks in future? It would make life easier I guess :)
(In reply to Alessandro Decina from comment #152)
> Firefox uses GStreamer on linux only for decoding audio (and video), but
> then uses custom output code. It doesn't use GStreamer sinks.

If I understand correctly, its means no hardware acceleration with gstreamer-vaapi ?
As I noted in bug 894372, the GStreamer 1.0 backend does autoplug vaapidecode if it's available, however CPU usage of Firefox is much higher here when it is: ~120% with vaapi vs. ~30% without, for Youtube 720p.
(In reply to antistress from comment #154)

> If I understand correctly, its means no hardware acceleration with
> gstreamer-vaapi ?

It means vaapi will be plugged if available, but will fallback to writing YUV buffers to main memory to hand them off to firefox. Full hw acceleration is certainly possible, but requires a little work.
(In reply to Alessandro Decina from comment #156)
> (In reply to antistress from comment #154)
> 
> > If I understand correctly, its means no hardware acceleration with
> > gstreamer-vaapi ?
> 
> It means vaapi will be plugged if available, but will fallback to writing
> YUV buffers to main memory to hand them off to firefox. Full hw acceleration
> is certainly possible, but requires a little work.

Thanks for the explanation
Depends on: 1024125
I want to run firefox *.html to play h264 video through HW accelerated path,but failed.Running gst-launch-0.10 playbin2 through HW accerelated path is OK. I need to upgrade gstreamer version to 1.0 ?
my environment parameters firefox32.01 html5 x86  ubuntu12.04  h264_1080p.mp4  gstremaer-0.10 gstreamer-vaapi-0.5.7 libva-1.3.1
The hardware path isn't supported in Firefox.
Thanks very much
Are there any plan to enable GStreamer 1.0 in Mozilla builds?
I'd like to know too, as i don't really have the old gstreamer anymore in places i'd like to use firefox.
Since this bug is closed, I opened a new request (bug #1159086) to specifically enable using GStreamer 1.0 by default, since Debian Jessie was released recently.
Blocks: 1160726
Still does not work in Firefox 40.x or SeaMonkey 2.35 - screenshot is here:
http://s15.postimg.org/5wehwu56j/Screenshot_from_2015_09_10_18_26_28.png
and attached. Those show Firefox 40.0.3 Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0 and SeaMonkey 2.35 Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 SeaMonkey/2.35 (both Mozilla builds - not distro builds), and Chrome Version 45.0.2454.85 (64-bit) using the same video as in my comment 147 of this bug report
https://bugzilla.mozilla.org/show_bug.cgi?id=806917#c147

Without gstreamer0.10-ffmpeg enabled the H.264 video does not play in Firefox and SeaMonkey. Only if I enable it does the video play. Note: my version is from gstreamer0.10-ffmpeg:
http://ppa.launchpad.net/mc3man/gstffmpeg-keep/ubuntu/ vivid/main amd64 Packages
(In reply to NoOp from comment #165)
> Still does not work in Firefox 40.x or SeaMonkey 2.35 

The actual issue to track for this is bug #947287. It's still not closed.
You need to log in before you can comment on or make changes to this bug.