Created attachment 319198 [details] [review]
audio: Move audioaggregator base class to a library

It's useful enough already to be used in other elements for audio aggregation,
let's give people the opportunity to use it and give it some API testing.

Created attachment 319199 [details] [review]
base/audio/video: Install headers and pkg-config files

They are still considered unstable API but it would be good to give them some
wider testing already to make sure the API is useful.

Created attachment 319200 [details] [review]
audioaggregator: Add vfunc that is called right before finishing an output buffer

Allows to do any final adjustments to the buffer before it's forwarded
downstream.

Review of attachment 319200 [details] [review]:

Looks like here the finish vfunc is not actually finishing, I guess you should rather have the ->finish vfunc default implementation do what we currently do outside the vmethod and ask subclasses to linkup.

Review of attachment 319199 [details] [review]:

OK

Review of attachment 319198 [details] [review]:

I do not see the file moves here, looks weird.

Review of attachment 319198 [details] [review]:

OK, splinter is just not representing files renaming it seems.

(In reply to Thibault Saunier from comment #4)
> Review of attachment 319200 [details] [review] [review]:
> 
> Looks like here the finish vfunc is not actually finishing, I guess you
> should rather have the ->finish vfunc default implementation do what we
> currently do outside the vmethod and ask subclasses to linkup.

Or should we maybe add something to GstAggregator that allows overriding finish()? This patch was not meant to go in here anyway, should've been in another bug :)

(In reply to Sebastian Dröge (slomo) from comment #8)
> (In reply to Thibault Saunier from comment #4)
> > Review of attachment 319200 [details] [review] [review] [review]:
> > 
> > Looks like here the finish vfunc is not actually finishing, I guess you
> > should rather have the ->finish vfunc default implementation do what we
> > currently do outside the vmethod and ask subclasses to linkup.
> 
> Or should we maybe add something to GstAggregator that allows overriding
> finish()? This patch was not meant to go in here anyway, should've been in
> another bug :)

ok, let's get the rest in and talk about that in another bug. It might indeed make sense to move that virtual method in GstAggregator itself.

Two things I'd like to see before freezing the APIs:

1. Have a ->negotiate() or similar vfunc in GstAggregator, and then have generic implementations in audioaggregator and videoaggregator. Both audio and video aggregator have code to move the handling of the caps event to the start of the ->aggregate() vfunc. Also, all the buffer pool/allocator "negotiation" code is currently done correctly in the glvideomixer class and the non-gl specific bits should go upwards to the base class.

2. The pad and element object locks are held across the call to ->aggregate_one_buffer() in GstAudioAggregator. It was meant as a temporary kludge, but I never got around to fixing it. We should probably release both before calling the vfunc, but we should make sure that the object state is still consistent afterwards.

(In reply to Olivier Crête from comment #10)
> Two things I'd like to see before freezing the APIs:

I do not think we are talking about freezing the API here, rather making it public even if unstable so that we can get more feedback.

Yes, I definitely don't want to freeze the API yet. GstAggregator might be ready, the audio/video ones still look unfinished to me and too closely modeled after their current subclasses. But that's exactly why I want to make them public so that we can know what people want to use them for and improve the API accordingly.

Created attachment 319543 [details] [review]
gst-plugins-bad-1.0: Ship badaudio/video/base libraries, headers and .pc files

Comment on attachment 319543 [details] [review]
gst-plugins-bad-1.0: Ship badaudio/video/base libraries, headers and .pc files

>     files_plugins_devel = [
>         'include/gstreamer-1.0/gst/gl',
>         'lib/gstreamer-1.0/include/gst/gl',
>         'include/gstreamer-1.0/gst/mpegts',
>         'include/gstreamer-1.0/gst/player',
>+        'include/gstreamer-1.0/gst/base',
>+        'include/gstreamer-1.0/gst/audio',
>+        'include/gstreamer-1.0/gst/video',
>         'lib/pkgconfig/gstreamer-gl-1.0.pc',
>         'lib/pkgconfig/gstreamer-mpegts-1.0.pc',
>-        'lib/pkgconfig/gstreamer-player-1.0.pc'
>+        'lib/pkgconfig/gstreamer-player-1.0.pc',
>+        'lib/pkgconfig/gstreamer-base-1.0.pc',
>+        'lib/pkgconfig/gstreamer-audio-1.0.pc',
>+        'lib/pkgconfig/gstreamer-video-1.0.pc'
>     ]

Surely the .pc files are called differently, something with -bad-*.pc ?

(I am not a fan of exposing these, just for the record)

Attachment 319198 [details] pushed as 8a1fa31 - audio: Move audioaggregator base class to a library
Attachment 319199 [details] pushed as c87a7e2 - base/audio/video: Install headers and pkg-config files

Attachment 319543 [details] pushed as 771988e - gst-plugins-bad-1.0: Ship badaudio/video/base libraries, headers and .pc files

This seems to break building the Android SDK. Steps:

./cerbero-uninstalled -c config/cross-android-armv7.cbc wipe
rm -rf ~/cerbero
git clean -d -f -x
git pull
./cerbero-uninstalled -c config/cross-android-armv7.cbc bootstrap
./cerbero-uninstalled -c config/cross-android-armv7.cbc package gstreamer-1.0

It failed with the following errors:

...
[(87/87) gst-editing-services-1.0-static -> fetch ]
[(87/87) gst-editing-services-1.0-static -> extract ]
[(87/87) gst-editing-services-1.0-static -> configure ]
[(87/87) gst-editing-services-1.0-static -> compile ]
[(87/87) gst-editing-services-1.0-static -> install ]
[(87/87) gst-editing-services-1.0-static -> post_install ]
WARNING: No specific packager available for the distro version android_gingerbread, using generic packager for distro android
-----> Creating package for gstreamer-1.0
***** Error running 'package' command:
The following files required by this package are missing:
 lib/pkgconfig/gstreamer-bad-base-1.0.pc
lib/pkgconfig/gstreamer-bad-video-1.0.pc
lib/pkgconfig/gstreamer-bad-audio-1.0.pc


After reverting 771988e the build completed successfully.

commit 355a580e9e23ad45fb33d6a43a231a1e25da9c30
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Jan 23 05:30:05 2016 +0200

    pkg-config: Properly version and install base/audio/video .pc files
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760733#c17