Build without gstreamer0.10

GStreamer 1.0 was released.
http://gstreamer.freedesktop.org/

GStreamer 1.0 has a different API and ABI as GStreamer 0.10,
but both versions should be usable in parallel for a while.

Announcement:
http://lists.freedesktop.org/archives/gstreamer-announce/2012-September/000265.html

GStreamer 0.10 to 1.0 porting guide:
http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/random/porting-to-1.0.txt

--
By by ... Detlef

Created attachment 48035
initial work toward gstreamer 1.0, non-working

So, following patch isn't working yet and might still be buggy, but it should be a good starting point toward gstreamer 1.0.

Notes about this patch:
- I think I've got all the caps ported, but somebody should recheck
- it seems that in the old code gst_pad_get_caps_reffed are used in gstdemux.c for no good reason (for the the _reffed part)
- the parts definitely done done yet are those for gst_pad_set_checkgetrange_function, gst_pad_set_bufferalloc_function, gst_pad_set_acceptcaps_function and gst_pad_set_setcaps_function (those need to be rewritten into queries, but I'm not sure how)
- as for that little bit around GST_SEEK_TYPE_CUR: given the notes in gstreamer 0.10 docs, I strongly suspect this is broken in the current code, though I'm not sure if my way is the proper fix (and it would be better to query position only once, but that would complicate the later code)

Anyway, before anything here is done for real, bug 30557 needs to be fixed.

Created attachment 48036
previous patch, corrected a bit

Just minor corrections of the previous version - no real changes, mostly stuff I've noticed thanks to colored diff of bugzilla.

Created attachment 48037
same patch, anothe minor corection

...OK, I think this is it for trivial changes...

Created attachment 48148
next version, still unworking

OK, I'm not sure some of these changes are correct, but it's almost done...
except for those bufferalloc functions, which I'm can't really say just how they need to be rewritten - that GST_QUERY_ALLOCATION note isn't really helpful.

@Rafał, are you still working on this?

(In reply to Michael Cronenworth from comment #5)
> @Rafał, are you still working on this?

Not really.

Given that:
- I don't know all that much about GStreamer for more than trivial ports
- I'm just about sure I've made mistakes in this patch
- some of the GStreamer 0.10 doc comments suggest that the current code is not quite correct (or at least not really working)
- I got not feedback from the devs here

I kind of given up.

...

PS: pity this wasn't given as a GSoC idea - it's about right complexity and size for it.

Hello, GStreamer developer here. If you have any particular questions about how to port some particular code, I'm happy to have a look. Do also feel free to pop into the #gstreamer IRC channel on freenode to ask questions. Some of the answers will depend on the context of the code in question of course; we don't know WINE very well or windows API, but if you point us to the code in git we should be able to figure it out.

Hi, Tim.

Besides the (yet to be addressed) problem from bug 30557, GStreamer use in wine is contained in dlls/winegstreamer/, with pretty much all but header includes constrained to the two files, those touched by my failed patch: dlls/winegstreamer/gstdemux.c and dlls/winegstreamer/gsttffilter.c.

While the doc situation has improved a little bit since the time I whined about it on IRC, there's still very little live code examples of allocation queries, mostly due to most gstreamer code redirecting the issue to handlers in abstract plugins (well, base classes, if you want to get technical).

While at this time there *is* a section in http://gstreamer.freedesktop.org/data/doc/gstreamer/head/pwg/html/section-allocation-query.html about it (as opposed to a TODO placeholder back when I was taking my shot), it still addresses it from video_buffer angle, while current gstreamer code in wine is for audio too. (would be nice if something could be done about bug 9127 too)

On a not really related note: kind of sorry, that I didn't pursue https://bugzilla.gnome.org/show_bug.cgi?id=734451 to its conclusion, but I though it'd be better to leave implementation details to you. While that problem affected people building gst-{ffmpeg,libav} against ffmpeg, while you (i.e. upstream) picked libav side of that silliness, it still would make the code more future proof, if - for example - libav decided to pick up one of (the admittedly few) codecs that have period or comma in its library name.

Rafał, do you have any *specific* questions about the ALLOCATION query? I think you can pretty much ignore it for a first port, especially for audio it's not really that useful at all. Just use gst_buffer_new*() and be done with it. It's mostly useful for video to avoid memory copies between decode and video sinks.

It seems I need to clarify a point - as soon as I came to conclusion, that I won't be able to port this on my own, I've pretty much dropped it. I'm chiming in here just to explain the history.

Having that said, from what little I remember about this code, gstreamer 0.10 here was using bufferalloc functions to prevent copying of allocated memory too.
Therefore gst_buffer_new*() might be insufficient, especially that this code needs to assign additional data to the buffer (IMediaSample pointer) that's being used in other callbacks.

Just to note that Debian started building WINE without GStreamer support because the plan is to drop GStreamer 0.10 from the next Debian release, see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785889 .
Maybe GStreamer support in WINE isn't critical?
What is lost from WINE if it is built without GStreamer?
Thanks!

(In reply to Ivan Baldo from comment #12)

> What is lost from WINE if it is built without GStreamer?

Winegstreamer doesn't work anyway because of bug 30557, and the workaround for apps that crash because of that bug is to disable winegstreamer, so AFAICT, nothing will be lost.

Some apps don't work without winegstreamer. The workaround for bug 30557 for those apps is to apply one of the patches linked in that bug that fixes the issue. Just disabling winegstreamer only turns one crash/nonworking bug into another crash/nonworking bug.

Some apps may fail gracefully without the support and just not play video (which is not ideal anyway), but others refuse to work properly if video playback fails. Both bugs need fixing.

@comment 14:

Well, of course *both* bugs need to be fixed, but till this one gets fixed, the fix for the other will get only minimal exposure in major distros, as the clock keeps ticking since EOS of 0.10.

Created attachment 53265
Patch sequence to update gstreamer usage to 1.0

(In reply to Andrew Eikum from comment #16)
> Created attachment 53265 [details]
> Patch sequence to update gstreamer usage to 1.0

This is a 5-patch sequence to update winegstreamer to use the gstreamer 1.0 API, based on Rafał's previous work. The first few patches are identical to those from Bug 30557.

While it seems to work in the few cases I tested it, it still needs lots of testing to verify that I didn't break anything.

The pixellation issue that Rosanne found with videos embedded in Powerpoint is also present with this patch series.

I forgot to mention, please be aware that there may be a problem with gstreamer 1.0 and seeking with MPGv1 videos. See https://bugzilla.gnome.org/show_bug.cgi?id=759976

Thanks for the credit, though I don't consider it fully earned.

Anyway, the patch builds - it might even work, but I'm not sure which apps were using it.

Only one I'm sure about isn't working (is just crashing) due to bug 9127...

I've tried to cargo-cult around it using some of YUV pieces, but it looks like I just don't sufficiently understand DirectShow.

(on that note: how do you get width/height/rate from MEDIASUBTYPE_MPEG1System ? you randomly pick one of the streams ?)

Though a couple minor points:

- it seems new GST_QUERY_ types should be added to switch, query_function, so that "Unknown query" are printed only for *unknown* values

- not quite sure if it's generic or app specific, but sometimes if a video fails to play due to a missing codec ("err:msvideo:ICLocate Required media codec 'vidc I420' not found!), on a retry the app crashes

- not sure if it can be helped, but I wonder if some kind of null input can be provided, so that the apps don't crash if a needed gst plugin isn't installed

Created attachment 53318
mciqtz32: Support MCI_DGV_PUT_DESTINATION

(In reply to Andrew Eikum from comment #17)
> The pixellation issue that Rosanne found with videos embedded in Powerpoint
> is also present with this patch series.

This patch fixes this problem in Office 2007.

(In reply to Rafał Mużyło from comment #19)
> Thanks for the credit, though I don't consider it fully earned.
>

I can remove the credit if you want, but I really did start with your patch and build on top of it. You had done much of the groundwork.

> Anyway, the patch builds - it might even work, but I'm not sure which apps
> were using it.
>

I've been testing mostly with a program called Graph Studio. Once that was working, I tested with GTA:VC, Echoes, Lego Designer, and PowerPoint and all seem to work.

> Though a couple minor points:
>
> - it seems new GST_QUERY_ types should be added to switch, query_function,
> so that "Unknown query" are printed only for *unknown* values
>

Sure, I'll make this a little better.

> - not quite sure if it's generic or app specific, but sometimes if a video
> fails to play due to a missing codec ("err:msvideo:ICLocate Required media
> codec 'vidc I420' not found!), on a retry the app crashes
>

I'll test the failure cases (missing gstreamer plugins) better and make sure we're handling the problem correctly.

> - not sure if it can be helped, but I wonder if some kind of null input can
> be provided, so that the apps don't crash if a needed gst plugin isn't
> installed

This could maybe be a future improvement.

Created attachment 53327
Rebased without new ntdll export

Here's another version without using the ntdll export as described in Bug 30557. This tarball also includes the fix for Powerpoint. Unless I hear of something broken or get review feedback, this is what I'll be submitting upstream soon.

(In reply to Andrew Eikum from comment #22)
> Created attachment 53327 [details]
> Rebased without new ntdll export
>
> Here's another version without using the ntdll export as described in Bug
> 30557. This tarball also includes the fix for Powerpoint. Unless I hear of
> something broken or get review feedback, this is what I'll be submitting
> upstream soon.

With those patches, when I try to insert either of the two videos, Powerpoint hangs with this in the console:

(wine:10444): GStreamer-WARNING **: Failed to load plugin '/usr/lib/gstreamer-1.0/libgstvideoparsersbad.so': /usr/lib/gstreamer-1.0/libgstvideoparsersbad.so: undefined symbol: gst_mpeg_video_meta_api_get_type

(wine:10444): GStreamer-WARNING **: Failed to load plugin '/usr/lib/gstreamer-1.0/libgstvdpau.so': /usr/lib/gstreamer-1.0/libgstvdpau.so: undefined symbol: gst_mpeg_video_meta_api_get_type
fixme:gstreamer:event_src 0x7a501e80 (56321) stub

** (wine:10444): CRITICAL **: gst_video_decoder_allocate_output_frame: assertion 'frame->output_buffer == NULL' failed
fixme:gstreamer:watch_bus mpegpsdemux0: Internal data stream error.
fixme:gstreamer:event_src 0x7a501e80 (56321) stub

I got past the errors in comment 23 by updating libgstcodecparsers. I can insert the videos in Powerpoint, but they don't play.

(In reply to Rosanne DiMesio from comment #24)
> I got past the errors in comment 23 by updating libgstcodecparsers. I can
> insert the videos in Powerpoint, but they don't play.

You use OpenSUSE, right? I'll install that in a VM and see if I can reproduce. In the meantime, can you upload a log with both

WINEDEBUG=+tid,+seh,+gstreamer,+quartz,+strmbase,+mci,+msvideo,+mciqtz

and

GST_DEBUG=9,GST_REFCOUNTING:0

set during the run?

Created attachment 53356
+tid,+seh,+gstreamer,+quartz,+strmbase,+mci,+msvideo,+mciqtz log

Attached is the log. I opened Powerpoint, inserted the Ikea video and set it to play automatically, then played the slide show.

I am on 64 bit openSUSE 13.1, using the Gstreamer 1.6.1 packages from the Packman repository.

Created attachment 53379
Update winegstreamer to 1.0, including better workaround for gst<1.7 bug

Thanks, Rosanne. The problem was caused by a bug that is fixed in gstreamer 1.7. I had a workaround for it in the patch, but it only fixed some situations. I've updated the workaround to be more robust, and this seems to fix the problem in PowerPoint for me. Can you try again?

(In reply to Andrew Eikum from comment #27)
> Created attachment 53379 [details]
> Update winegstreamer to 1.0, including better workaround for gst<1.7 bug
>
> Thanks, Rosanne. The problem was caused by a bug that is fixed in gstreamer
> 1.7. I had a workaround for it in the patch, but it only fixed some
> situations. I've updated the workaround to be more robust, and this seems to
> fix the problem in PowerPoint for me. Can you try again?

Oh, note that pausing/restarting videos in powerpoint seems in poor shape. See Bug 30687. But I was at least able to get the video preview to work, and it worked in a slideshow if I didn't try to restart it after pausing.

(In reply to Andrew Eikum from comment #28)

>
> Oh, note that pausing/restarting videos in powerpoint seems in poor shape.
> See Bug 30687. But I was at least able to get the video preview to work, and
> it worked in a slideshow if I didn't try to restart it after pausing.

Other than the pause/restart issue, it works great.

Wine is now using the gstreamer 1.0 API. Please file new bugs if you encounter problems with our gstreamer support. I'll send a mail to the mailing lists to update packagers about the dependency change.

commit e8311270ab7e01b8c58ec615f039335bd166882a
Author: Andrew Eikum <email address hidden>
Date: Thu Jan 14 13:23:04 2016 -0600

    winegstreamer: Update to use gstreamer-1.0.

Just before this bug gets closed, a question:
What exactly is the purpose of
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <gst/gst.h>]], [[static int a[sizeof(gint64) > 4 ? 1 : -1]; if (a[0]) return 0;]])

check ?

Cause, unless I'm missing something, that check was bogus even before glib 2.0.0.

The above question is wrt. some of the responses to the mail to the packagers, where I strongly suspect the errors are simply cases of incorrect toolchain setup.

(In reply to Rafał Mużyło from comment #31)
> Just before this bug gets closed, a question:
> What exactly is the purpose of
> AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <gst/gst.h>]], [[static int
> a[sizeof(gint64) > 4 ? 1 : -1]; if (a[0]) return 0;]])
>
> check ?
>
> Cause, unless I'm missing something, that check was bogus even before glib
> 2.0.0.

The problem here is glib and gstreamer have different header files for 32- and 64-bit, but pkgconfig doesn't have a mechanism to specify which architecture's headers you should get. This is really glib/gstreamer's fault. The vast majority of libraries write their headers so the same header files work on either architecture, and there is no need to configure pkg-config.

For most (all?) distros, the 64-bit glib/gstreamer headers are in the default location, while the 32-bit ones are in some special location (in /usr/lib32/ on Arch Linux). So pkg-config always gives the 64-bit headers, even when building 32-bit Wine, and this results in broken winegstreamer.

That check was added in b113af1b13455 to avoid building gstreamer if the headers are for the wrong architecture.

This was discussed a bit on the ML in this thread:

https://www.winehq.org/pipermail/wine-devel/2016-January/111245.html

(In reply to Andrew Eikum from comment #33)
> The problem here is glib and gstreamer have different header files for 32-
> and 64-bit, but pkgconfig doesn't have a mechanism to specify which
> architecture's headers you should get. This is really glib/gstreamer's
> fault. The vast majority of libraries write their headers so the same header
> files work on either architecture, and there is no need to configure
> pkg-config.
>

Not quite correct and Gentoo here, so I kind of know about arch-specific headers. While almost exclusively a spectator, I did watch the growing pains of multilib-minimal and related eclases and did notice some packages started wrapping some of their headers (those, that weren't already using /usr/lib{32,64} for that purpose). While some of the used solutions weren't technically quite correct (like passing '-m32' as a tail of CC/CXX instead of in C{,XX}FLAGS), sme were quite interesting.

> For most (all?) distros, the 64-bit glib/gstreamer headers are in the
> default location, while the 32-bit ones are in some special location (in
> /usr/lib32/ on Arch Linux). So pkg-config always gives the 64-bit headers,
> even when building 32-bit Wine, and this results in broken winegstreamer.
>

...unless you've got a real 32bit machine or using such vm...but kind of off-topic.
Anyway, even if you don't have a host-prefixed pkg-config (which is the most simple way of making pkg-config "just work"), there's still PKG_CONFIG_LIBDIR, which, again, is a mater of a proper toolchain setup.

> That check was added in b113af1b13455 to avoid building gstreamer if the
> headers are for the wrong architecture.
>

That check is nevertheless bogus - sizeof(gint64) by design is *always* 8.

> This was discussed a bit on the ML in this thread:
>
> https://www.winehq.org/pipermail/wine-devel/2016-January/111245.html

...and this was the thread I was referring to when I've said "the errors are simply cases of incorrect toolchain setup".

...

I'm not completely dismissing the mail you've referenced, but saying that the content of the relevant config.log might be slightly different than you'd expect.

(In reply to Rafał Mużyło from comment #34)
> That check is nevertheless bogus - sizeof(gint64) by design is *always* 8.
>

With the correct glib headers for your architecture, yes, it is always 8. But without setting PKG_CONFIG_PATH to pick up the correct glib-2.0.pc file, it will be wrong. This is what the configure check is for.

[aeikum@aeikum ~]$ cat test.c
#include <stdio.h>
#include <glib-2.0/glib.h>

int main(int argc, char **argv)
{
    printf("%u\n", sizeof(gint64));
    return 0;
}
[aeikum@aeikum ~]$ gcc $(pkg-config --cflags --libs glib-2.0) -o test test.c
[aeikum@aeikum ~]$ ./test
8
[aeikum@aeikum ~]$ gcc -m32 $(pkg-config --cflags --libs glib-2.0) -o test test.c
[aeikum@aeikum ~]$ ./test
4
[aeikum@aeikum ~]$ gcc -m32 $(PKG_CONFIG_PATH=/usr/lib32/pkgconfig pkg-config --cflags --libs glib-2.0) -o test test.c
[aeikum@aeikum ~]$ ./test
8
[aeikum@aeikum ~]$

The way I read wine's configure.ac and glibconfig.h, the check you really want here is slightly different and in a slightly different position.

Namely, first the checks AC_CHECK_HEADER/AC_CHECK_LIB should run.
Then, if they succeed, the next check to run would be AC_COMPILE_IFELSE for 'GLIB_SIZEOF_LONG == sizeof(long)'.
If that fails, you want to AC_MSG_ERROR right here, cause this would mean exactly that the mentioned toolchain mistake took place.

(In reply to Rafał Mużyło from comment #37)
> The way I read wine's configure.ac and glibconfig.h, the check you really
> want here is slightly different and in a slightly different position.
>
> Namely, first the checks AC_CHECK_HEADER/AC_CHECK_LIB should run.
> Then, if they succeed, the next check to run would be AC_COMPILE_IFELSE for
> 'GLIB_SIZEOF_LONG == sizeof(long)'.
> If that fails, you want to AC_MSG_ERROR right here, cause this would mean
> exactly that the mentioned toolchain mistake took place.

I'd happily approve of a patch to improve this detection and its diagnostic message. I'd probably go with a warning instead of an error, since many distros don't ship 32-bit gstreamer and I don't want to kill those builds because they happen to have 64-bit gstreamer installed.

...ah, OK, now I see how that exactly works...

Kind of fragile.

Still, what I've wrote earlier about the conditions of the check holds - the mismatch should AC_MSG_ERROR, as it indicates an incorrect toolchain setup.

> I'd probably go with a warning instead of an error, since many distros don't ship 32-bit gstreamer and I don't want to kill those builds because they happen to have 64-bit gstreamer installed.

This is the part we keep misunderstanding each other - I don't see how you could construct such case.

I'd put AC_MSG_ERROR only in case gstreamer is detected, but is mismatched.

If it's not detected, we're not in the branch.

If we're building wine64, gstreamer is detected and mismatch happens, toolchain is incorrect.

If we're building wine32, gstreamer is detected and mismatch happens, toolchain is incorrect.

(In reply to Rafał Mużyło from comment #40)
> > I'd probably go with a warning instead of an error, since many distros don't ship 32-bit gstreamer and I don't want to kill those builds because they happen to have 64-bit gstreamer installed.
>
> This is the part we keep misunderstanding each other - I don't see how you
> could construct such case.
>
> I'd put AC_MSG_ERROR only in case gstreamer is detected, but is mismatched.
>
> If it's not detected, we're not in the branch.
>

You're right. I was thinking the headers were sufficient to get into the branch, but the error will only occur if 32-bit gstreamer SO files are present. Do you want to write a patch, or shall I add this to my TODO list?

Aborting configure with AC_MSG_ERROR is very unfriendly, and should only be used for cases that would make the resulting build useless. A broken gstreamer doesn't qualify. A simple notice message would be enough.

(In reply to Alexandre Julliard from comment #42)
> Aborting configure with AC_MSG_ERROR is very unfriendly, and should only be
> used for cases that would make the resulting build useless. A broken
> gstreamer doesn't qualify. A simple notice message would be enough.

The catch is next to noone will notice such notice in the flood of the configure messages, unless this check will only set a var and the warning is only printed at the conclusion of the configure process.

That's not about my workflow, but about my experience with people's approach to the content of build logs, non-failing configure parts specifically. That's the common variant of "if it builds, it works".

That's just a personal preference, but I'd prefer to abort as soon as I know something is broken, cause if I care, I want to fix it and if I don't, I'd know I'd need to add '--disable-gstreamer'.

With your approach, you might just as well scrap the check altogether and build a broken gstreamer in such case - the resulting build wouldn't be useless then, just the gstreamer part.

Created attachment 53467
minor configure check tweak

This is basically what I was talking about.
Mind I haven't tested it, so I might have mismatched the braces, but it's more to show the concept (the message is badly worded too).

As I've said, instead of dieing, it could set a var here and report it at the end of the process, but again, in my experience, non-dieing *fails* are a mistake.

(In reply to Rafał Mużyło from comment #43)
> The catch is next to noone will notice such notice in the flood of the
> configure messages, unless this check will only set a var and the warning is
> only printed at the conclusion of the configure process.

That's how it's done for all such warnings. Check the WINE_NOTICE macro.

Closing bugs fixed in 1.9.2.

Rafał, commit 049412a48c should help users with this problem.