Last Comment Bug 806917 - support GStreamer 1.0
: support GStreamer 1.0
Status: RESOLVED FIXED
: feature
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All Linux
: -- enhancement with 35 votes (vote)
: mozilla30
Assigned To: Alessandro Decina
: Alexandra Lucinet, QA Mentor [:adalucinet]
Mentors:
: 971609 972192 (view as bug list)
Depends on: 878363 928806 952769 989112 1024125
Blocks: 794282 833628 851290 947287 971609 1160726
  Show dependency treegraph
 
Reported: 2012-10-30 06:45 PDT by Wolfgang Rosenauer [:wolfiR]
Modified: 2015-09-10 19:19 PDT (History)
105 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
affected
30+


Attachments
First pass at a patch. (22.32 KB, patch)
2013-02-16 12:32 PST, Mike Gorse
no flags Details | Diff | Review
Work in progress. (25.14 KB, patch)
2013-03-26 09:57 PDT, Mike Gorse
no flags Details | Diff | Review
WIP 1.0 patch (46.45 KB, patch)
2013-04-08 02:45 PDT, Alessandro Decina
no flags Details | Diff | Review
WIP 1.0 support (56.77 KB, patch)
2013-05-07 02:38 PDT, Alessandro Decina
no flags Details | Diff | Review
sadface pipeline dump (82.20 KB, image/svg+xml)
2013-05-10 18:38 PDT, Edwin Flores [:eflores] [:edwin]
no flags Details
1.0 support (56.34 KB, text/x-patch)
2013-05-11 13:05 PDT, Alessandro Decina
no flags Details
1.0 support (56.35 KB, patch)
2013-05-12 04:39 PDT, Alessandro Decina
no flags Details | Diff | Review
Small fixes (3.88 KB, patch)
2013-05-12 21:10 PDT, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Review
WIP (71.12 KB, patch)
2013-08-11 16:12 PDT, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Review
0001-Bug-806917-support-GStreamer-1.0.patch (52.18 KB, patch)
2013-10-21 01:01 PDT, Alessandro Decina
no flags Details | Diff | Review
806917-nov-fixes.patch (22.20 KB, patch)
2013-11-11 17:11 PST, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Review
new patch (79.91 KB, patch)
2013-12-15 06:38 PST, Alessandro Decina
no flags Details | Diff | Review
bug806917.patch (80.17 KB, patch)
2013-12-18 01:47 PST, Alessandro Decina
no flags Details | Diff | Review
new version (79.64 KB, patch)
2013-12-23 14:25 PST, Alessandro Decina
no flags Details | Diff | Review
rebased version after gfx::IntSize changes in m-c (79.65 KB, patch)
2014-01-03 08:16 PST, Alessandro Decina
no flags Details | Diff | Review
1.0.patch (81.95 KB, patch)
2014-01-22 22:46 PST, Alessandro Decina
no flags Details | Diff | Review
1.0.patch (81.96 KB, patch)
2014-01-23 17:03 PST, Alessandro Decina
edwin: review+
gps: review+
Details | Diff | Review
rebased (81.97 KB, patch)
2014-02-07 14:40 PST, Alessandro Decina
no flags Details | Diff | Review
rebased 1.0 patch (81.98 KB, patch)
2014-02-10 19:43 PST, Alessandro Decina
no flags Details | Diff | Review
disable-gstreamer.patch (389 bytes, patch)
2014-02-11 10:59 PST, Jan Steffens
gps: review+
dholbert: feedback+
Details | Diff | Review
Screenshot from 2014-02-18 17:08:17.png (99.89 KB, image/png)
2014-02-18 14:10 PST, Andrew Overholt [:overholt]
no flags Details
Screenshot of FF and SM without gstreamer0.10-ffmpeg installed (470.74 KB, image/png)
2015-09-10 19:09 PDT, NoOp
no flags Details

Description Wolfgang Rosenauer [:wolfiR] 2012-10-30 06:45:09 PDT
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
Comment 1 Mike Gorse 2013-02-16 12:32:52 PST
Created attachment 714832 [details] [diff] [review]
First pass at a patch.

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).
Comment 2 Chris Pearce (:cpearce) 2013-02-18 13:31:39 PST
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.
Comment 3 Mike Gorse 2013-03-26 09:57:10 PDT
Created attachment 729607 [details] [diff] [review]
Work in progress.

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).
Comment 4 Alessandro Decina 2013-04-08 02:45:22 PDT
Created attachment 734530 [details] [diff] [review]
WIP 1.0 patch

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).
Comment 5 Alessandro Decina 2013-05-07 02:38:30 PDT
Created attachment 746297 [details] [diff] [review]
WIP 1.0 support

Seeking works now and I started moving the 0.10 specific code to GStreamerReader-0.10.cpp
Comment 6 Edwin Flores [:eflores] [:edwin] 2013-05-10 18:38:12 PDT
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.
Comment 7 Edwin Flores [:eflores] [:edwin] 2013-05-10 18:38:50 PDT
Created attachment 748341 [details]
sadface pipeline dump
Comment 8 Edwin Flores [:eflores] [:edwin] 2013-05-10 19:35:41 PDT
(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.
Comment 9 Alessandro Decina 2013-05-11 12:45:28 PDT
(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?
Comment 10 Alessandro Decina 2013-05-11 13:05:22 PDT
Created attachment 748471 [details]
1.0 support

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.
Comment 11 Alessandro Decina 2013-05-12 04:39:09 PDT
Created attachment 748518 [details] [diff] [review]
1.0 support

Same as the previous patch, with a fix in configure.in so that --enable-gstreamer=X actually works
Comment 12 Edwin Flores [:eflores] [:edwin] 2013-05-12 16:22:35 PDT
(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
Comment 13 Edwin Flores [:eflores] [:edwin] 2013-05-12 21:10:19 PDT
Created attachment 748637 [details] [diff] [review]
Small fixes

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
Comment 14 Alessandro Decina 2013-05-13 00:36:29 PDT
(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
Comment 15 Edwin Flores [:eflores] [:edwin] 2013-05-13 00:45:56 PDT
I'm using 1.0.5. I think most distros are on 1.0.x, so it's the most desirable target for now.
Comment 16 Mike Hommey [:glandium] 2013-05-13 01:34:07 PDT
(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.
Comment 17 pbrobinson 2013-05-13 01:47:05 PDT
> 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.
Comment 18 Mike Hommey [:glandium] 2013-05-13 02:07:38 PDT
(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.
Comment 19 Mike Hommey [:glandium] 2013-05-13 02:08:45 PDT
(In reply to Mike Hommey [:glandium] from comment #18)
> (and that gtk uses too, iirc)

That would be libgnome.
Comment 20 Mike Hommey [:glandium] 2013-05-13 02:12:37 PDT
(That's why bug 859199 is essential).
Comment 21 Edwin Flores [:eflores] [:edwin] 2013-05-15 17:40:59 PDT
(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.
Comment 22 Alex Xu 2013-06-19 15:57:16 PDT
What's the progress on this now?
Comment 24 Giuliano Lotta 2013-07-19 14:27:39 PDT
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 ?
Comment 25 Ralph Giles (:rillian) needinfo me 2013-07-19 14:59:39 PDT
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.
Comment 26 Giuliano Lotta 2013-07-19 16:47:46 PDT
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 ?
Comment 27 Hussam Al-Tayeb 2013-07-19 16:58:46 PDT
(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.
Comment 28 Giuliano Lotta 2013-07-20 14:06:19 PDT
And the patch itself for which version (= or >= )is realized ?
Comment 29 Edwin Flores [:eflores] [:edwin] 2013-08-11 16:12:09 PDT
Created attachment 788730 [details] [diff] [review]
WIP

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.
Comment 30 Edwin Flores [:eflores] [:edwin] 2013-08-11 16:33:57 PDT
(In reply to Edwin Flores [:eflores] [:edwin] from comment #29)
> Outstanding issues:
There is also still a shutdown hang sometimes.
Comment 31 dE 2013-09-18 21:44:17 PDT
This's going to help with va-api also.
Comment 32 Jan Steffens 2013-09-24 06:13:37 PDT
1.2 has been released. Should this be the target instead? Seems to be mostly compatible with 1.0.
Comment 33 seppo.yli-olli 2013-10-16 04:38:27 PDT
(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.
Comment 34 Alessandro Decina 2013-10-16 05:09:30 PDT
(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.
Comment 35 Alessandro Decina 2013-10-21 01:01:36 PDT
Created attachment 819584 [details] [diff] [review]
0001-Bug-806917-support-GStreamer-1.0.patch

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.
Comment 36 Edwin Flores [:eflores] [:edwin] 2013-11-05 17:13:08 PST
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.
Comment 37 antistress 2013-11-05 17:19:51 PST
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!
Comment 38 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2013-11-05 17:46:38 PST
Hardware H.264 support through VA-API is possible but not yet supported. Hardware decoding of VP8 is not supported by VA-API.
Comment 39 Edwin Flores [:eflores] [:edwin] 2013-11-10 17:00:21 PST
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?).
Comment 40 Edwin Flores [:eflores] [:edwin] 2013-11-11 17:11:27 PST
Created attachment 830538 [details] [diff] [review]
806917-nov-fixes.patch

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 41 Edwin Flores [:eflores] [:edwin] 2013-11-11 17:12:22 PST
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.
Comment 42 Alessandro Decina 2013-11-16 05:51:10 PST
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
Comment 48 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2013-12-08 13:10:23 PST
*** Bug 947287 has been marked as a duplicate of this bug. ***
Comment 49 Alessandro Decina 2013-12-15 06:38:41 PST
Created attachment 8347734 [details] [diff] [review]
new patch

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.
Comment 50 Alessandro Decina 2013-12-15 08:37:15 PST
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.
Comment 51 Sebastian Dröge (:slomo) 2013-12-15 09:42:59 PST
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 52 Chris Pearce (:cpearce) 2013-12-15 12:19:54 PST
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.
Comment 53 Alex Xu 2013-12-15 12:24:17 PST
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
Comment 54 Alessandro Decina 2013-12-15 12:52:37 PST
(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.
Comment 55 Alessandro Decina 2013-12-15 12:53:37 PST
(In reply to Alex Xu from comment #53)
> With --enable-warnings-as-errors and clang:
[snip]

Fixed both warnings locally. Thanks!
Comment 56 Edwin Flores [:eflores] [:edwin] 2013-12-17 15:17:59 PST
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.
Comment 57 Alessandro Decina 2013-12-18 01:47:40 PST
Created attachment 8349295 [details] [diff] [review]
bug806917.patch
Comment 58 Alessandro Decina 2013-12-18 01:51:49 PST
(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
Comment 59 Alessandro Decina 2013-12-21 08:29:01 PST
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.
Comment 60 Alessandro Decina 2013-12-23 14:25:39 PST
Created attachment 8351261 [details] [diff] [review]
new version

Ok, this version + https://bugzilla.mozilla.org/show_bug.cgi?id=952769 should fix the stride issue with 1.0 and accelerated compositing
Comment 61 Alex Xu 2014-01-02 08:41:24 PST
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.
Comment 62 Alex Xu 2014-01-02 19:01:38 PST
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
Comment 63 Alessandro Decina 2014-01-03 08:16:54 PST
Created attachment 8355558 [details] [diff] [review]
rebased version after gfx::IntSize changes in m-c
Comment 64 Edwin Flores [:eflores] [:edwin] 2014-01-06 16:27:40 PST
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 65 Edwin Flores [:eflores] [:edwin] 2014-01-22 17:23:49 PST
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.
Comment 66 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2014-01-22 18:19:47 PST
Comment on attachment 8355558 [details] [diff] [review]
rebased version after gfx::IntSize changes in m-c

302 not me
Comment 67 Alessandro Decina 2014-01-22 22:46:41 PST
Created attachment 8364193 [details] [diff] [review]
1.0.patch

review fixes
Comment 68 Alessandro Decina 2014-01-22 22:54:54 PST
(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.
Comment 69 Alex Xu 2014-01-23 06:36:41 PST
(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.
Comment 70 Alessandro Decina 2014-01-23 17:03:54 PST
Created attachment 8364791 [details] [diff] [review]
1.0.patch

Fixed a few more nsIntSize => gfx::IntSize occurrencies

https://tbpl.mozilla.org/?tree=Try&rev=b1ab12fdce27
Comment 71 Alessandro Decina 2014-01-29 14:31:59 PST
ping? :)
Comment 72 Edwin Flores [:eflores] [:edwin] 2014-01-29 23:09:11 PST
Comment on attachment 8364791 [details] [diff] [review]
1.0.patch

LGTM
Comment 73 Alex Xu 2014-01-30 08:55:34 PST
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.
Comment 74 Alex Xu 2014-01-30 11:53:38 PST
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 75 Gregory Szorc [:gps] 2014-01-30 17:07:23 PST
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"?
Comment 76 Alex Xu 2014-01-30 17:39:49 PST
(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.
Comment 77 Alessandro Decina 2014-01-30 18:42:31 PST
(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).
Comment 78 Alex Xu 2014-01-31 08:46:20 PST
(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.
Comment 79 Bastien Nocera 2014-01-31 08:50:16 PST
(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
Comment 80 Alex Xu 2014-02-04 17:39:55 PST
(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".
Comment 82 Alex Xu 2014-02-08 10:15:56 PST
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. (?)
Comment 83 Alex Xu 2014-02-08 17:21:13 PST
Actually, I think this is in mozilla-central, not your patch.
Comment 84 Jan Beich 2014-02-09 06:05:46 PST
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()?
Comment 85 Alessandro Decina 2014-02-10 19:43:43 PST
Created attachment 8373818 [details] [diff] [review]
rebased 1.0 patch

Alex, this should fix the MAX_CHANNELS warning.

https://tbpl.mozilla.org/?tree=Try&rev=5e6f274b19df
Comment 86 Alessandro Decina 2014-02-11 00:09:08 PST
https://tbpl.mozilla.org/?tree=Try&rev=5e6f274b19df
Comment 87 Alessandro Decina 2014-02-11 00:14:06 PST
(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.
Comment 88 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-02-11 00:34:17 PST
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.
Comment 89 Ryan VanderMeulen [:RyanVM] 2014-02-11 06:24:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/63cdfb958a52
Comment 90 Luis de Bethencourt [:luisbg] 2014-02-11 07:12:41 PST
I just tested Alessandro's patch and it works.
\o/
Comment 91 Daniel Holbert [:dholbert] 2014-02-11 10:28:11 PST
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.
Comment 92 Daniel Holbert [:dholbert] 2014-02-11 10:32:08 PST
(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.))
Comment 93 Daniel Holbert [:dholbert] 2014-02-11 10:33:19 PST
gps, do you know what might be going on with Comment 91?
Comment 94 Luis de Bethencourt [:luisbg] 2014-02-11 10:39:56 PST
I can reproduce Comment 91 following the same steps as Daniel points out.
Comment 95 Jan Steffens 2014-02-11 10:59:47 PST
Created attachment 8374293 [details] [diff] [review]
disable-gstreamer.patch

Here's a quick, untested fix to handle --disable-gstreamer properly.
Comment 96 Daniel Holbert [:dholbert] 2014-02-11 11:07:30 PST
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?
Comment 97 Daniel Holbert [:dholbert] 2014-02-11 11:08:37 PST
(This explains the "no" suffix in "gstreamer-app-no" and "gstreamer-plugins-base-no" from comment 91)
Comment 98 Ryan VanderMeulen [:RyanVM] 2014-02-11 11:51:14 PST
https://hg.mozilla.org/mozilla-central/rev/63cdfb958a52
Comment 99 Scott Baker 2014-02-11 12:57:24 PST
Just for clarification, does this mean that nightly builds will now support the GStreamer backend, and whatever codecs it supports?
Comment 100 Ralph Giles (:rillian) needinfo me 2014-02-11 13:00:59 PST
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.
Comment 101 Luis de Bethencourt [:luisbg] 2014-02-11 13:06:11 PST
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.
Comment 102 Jan Steffens 2014-02-11 13:08:53 PST
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.
Comment 103 Luis de Bethencourt [:luisbg] 2014-02-11 13:28:55 PST
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.
Comment 104 Jan Steffens 2014-02-11 13:47:33 PST
(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.
Comment 105 Luis de Bethencourt [:luisbg] 2014-02-11 13:51:52 PST
That is what I meant, I agree with you. Maybe I miscommunicated :)
Comment 106 Alessandro Decina 2014-02-12 00:34:38 PST
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!
Comment 107 Scott Baker 2014-02-12 08:02:00 PST
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 108 Daniel Holbert [:dholbert] 2014-02-12 10:43:44 PST
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.]
Comment 109 Gregory Szorc [:gps] 2014-02-12 18:04:08 PST
Comment on attachment 8374293 [details] [diff] [review]
disable-gstreamer.patch

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

Oy. Configure can be fickle.
Comment 110 Daniel Holbert [:dholbert] 2014-02-12 18:19:12 PST
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!
Comment 111 Daniel Holbert [:dholbert] 2014-02-12 18:20:11 PST
*** Bug 971609 has been marked as a duplicate of this bug. ***
Comment 112 Ryan VanderMeulen [:RyanVM] 2014-02-13 07:35:41 PST
https://hg.mozilla.org/mozilla-central/rev/02f0327bf8e1
Comment 113 Luis de Bethencourt [:luisbg] 2014-02-13 07:41:40 PST
Kudos to Alessandro for all this cool work :)
Comment 114 Ryan VanderMeulen [:RyanVM] 2014-02-13 07:47:32 PST
*** Bug 972192 has been marked as a duplicate of this bug. ***
Comment 115 Alessandro Decina 2014-02-13 15:44:21 PST
(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?
Comment 116 Luis de Bethencourt [:luisbg] 2014-02-13 15:57:14 PST
Alessandro,

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

Sounds good?
Comment 117 Alessandro Decina 2014-02-13 16:02:32 PST
(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/
Comment 118 Luis de Bethencourt [:luisbg] 2014-02-13 16:11:04 PST
Done
Comment 119 vulcain 2014-02-14 06:17:17 PST
There are a backport to Firefox Beta 28 or Aurora 29 ??
Comment 120 Alex Xu 2014-02-14 07:00:40 PST
Uplift is not likely to be accepted AFAIK; this is a major change which has fairly large risks and minor benefits.
Comment 121 vulcain 2014-02-14 09:19:02 PST
(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.
Comment 122 Andrew Overholt [:overholt] 2014-02-18 14:10:20 PST
Created attachment 8377833 [details]
Screenshot from 2014-02-18 17:08:17.png

(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.
Comment 123 Sylvestre Ledru [:sylvestre] 2014-03-06 01:32:50 PST
Added in the release note database for 30.
Comment 124 Johnathan 2014-03-16 03:20:35 PDT
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?
Comment 125 Alessandro Decina 2014-03-16 03:26:40 PDT
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.
Comment 126 Ralf Jung 2014-04-05 04:19:40 PDT
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.
Comment 127 Cork 2014-04-05 08:35:00 PDT
(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.
Comment 128 Ralf Jung 2014-04-06 01:32:14 PDT
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.
Comment 129 Sebastian Dröge (:slomo) 2014-04-15 12:09:38 PDT
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.
Comment 130 Luis de Bethencourt [:luisbg] 2014-04-15 12:16:05 PDT
+1 to changing the build defaults to GStreamer 1.x.
Comment 131 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-04-15 14:02:23 PDT
1.0 isn't available on Debian Stable.

https://packages.debian.org/search?keywords=libgstreamer&searchon=names&suite=stable&section=all
Comment 132 Kevin Cox [:kevincox] 2014-04-15 14:08:57 PDT
> 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.
Comment 133 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-04-15 14:36:10 PDT
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.
Comment 134 Mike Hommey [:glandium] 2014-04-15 15:01:26 PDT
Can't we support both? we're dlopening it, there's no reason we can't.
Comment 135 Ralf Jung 2014-04-16 00:08:34 PDT
(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>
Comment 136 Shmerl 2014-05-18 18:55:23 PDT
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
Comment 137 Shmerl 2014-05-18 18:56:57 PDT
[sorry, the comment got submitted partially - contd.]:

So making gstreamer 0.10 the default in Firefox because of Debian doesn't make sense.
Comment 139 Sebastian Dröge (:slomo) 2014-05-18 23:25:18 PDT
The issue is not the builds from the distro, but the official builds from Mozilla.
Comment 140 Ralf Jung 2014-05-19 00:41:03 PDT
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.
Comment 141 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-05-20 21:02:57 PDT
I use http://mozilla.debian.net/
Comment 142 Mike Hommey [:glandium] 2014-05-20 21:09:18 PDT
(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.
Comment 143 dE 2014-05-20 22:10:22 PDT
How do I enable hardware acceleration with GST in FF?
Comment 144 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-05-20 23:40:15 PDT
Hardware acceleration is not supported.
Comment 145 antistress 2014-05-21 02:10:43 PDT
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
Comment 146 dE 2014-05-21 03:54:26 PDT
(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.
Comment 147 NoOp 2014-06-12 23:42:33 PDT
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
Comment 148 Shmerl 2014-06-13 07:28:24 PDT
(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).
Comment 149 nucrap 2014-06-14 17:52:46 PDT
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?
Comment 150 Shmerl 2014-06-14 19:32:27 PDT
(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.
Comment 151 nucrap 2014-06-15 04:14:07 PDT
@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.
Comment 152 Alessandro Decina 2014-06-15 04:34:25 PDT
(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.
Comment 153 nucrap 2014-06-15 04:46:40 PDT
(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 :)
Comment 154 antistress 2014-06-15 05:14:01 PDT
(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 ?
Comment 155 Jan Steffens 2014-06-15 05:16:40 PDT
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.
Comment 156 Alessandro Decina 2014-06-15 06:30:44 PDT
(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.
Comment 157 antistress 2014-06-15 08:04:02 PDT
(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
Comment 158 Yilong Zhou 2014-10-19 19:24:06 PDT
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 ?
Comment 159 Yilong Zhou 2014-10-19 20:03:34 PDT
my environment parameters firefox32.01 html5 x86  ubuntu12.04  h264_1080p.mp4  gstremaer-0.10 gstreamer-vaapi-0.5.7 libva-1.3.1
Comment 160 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-10-19 21:17:43 PDT
The hardware path isn't supported in Firefox.
Comment 161 Yilong Zhou 2014-10-19 22:42:30 PDT
Thanks very much
Comment 162 b.pagani 2015-01-19 02:22:37 PST
Are there any plan to enable GStreamer 1.0 in Mozilla builds?
Comment 163 Johnny Robeson 2015-01-19 02:27:30 PST
I'd like to know too, as i don't really have the old gstreamer anymore in places i'd like to use firefox.
Comment 164 Shmerl 2015-04-27 18:51:45 PDT
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.
Comment 165 NoOp 2015-09-10 19:06:05 PDT
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
Comment 166 NoOp 2015-09-10 19:09:13 PDT
Created attachment 8659676 [details]
Screenshot of FF and SM without gstreamer0.10-ffmpeg installed
Comment 167 Shmerl 2015-09-10 19:19:32 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.