On Dec 1, 2007 1:17 PM, Valery Masiutsin <val.masutin at gmail.com>
wrote:> Hello, list !
>
> Here is diff of changes, i had to make to get swfdec running on
> OpenBSD 4.2-current.
>
> Regards Valery.
Hey,
I'm sorry for not getting to this earlier. Brad reminded me of this
today. Anyway, here's a detailed review of the patch:
> diff --git a/autogen.sh b/autogen.sh
> index 3006db5..ed64391 100755
> --- a/autogen.sh
> +++ b/autogen.sh
> @@ -1,3 +1,3 @@
> #!/bin/sh
> autoreconf -i -f &&
> -./configure --enable-maintainer-mode --disable-static --enable-gtk-doc
--enable-vivified --enable-ffmpeg --enable-mad $@
> +./configure --enable-maintainer-mode --disable-static --enable-gtk-doc
--disable-vivified --enable-ffmpeg --disable-gstreamer --enable-mad
--with-audio=oss --disable-gtk-doc $@
>
I guess this is clearly a config issue.
A build should use autoreconf -i -f && ./configure instead of
autogen.sh in any case. autogen.sh at this point enables features we
expect Swfdec developers to use, but not average users. (Yes, I know
that is non-standard, better ideas on where to put developer flags
welcome).
> diff --git a/configure.ac b/configure.ac
> index bb86e8f..fd241da 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -163,7 +163,11 @@ dnl Assume OSS is available if ALSA wasn't found
and we're "auto".
> if test "$with_audio" = "auto" -o
"$with_audio" = "oss"; then
> with_audio="oss"
> AUDIO_CFLAGS> - AUDIO_LIBS> + if test `uname` =
"OpenBSD" ; then
> + AUDIO_LIBS=-lossaudio
> + else
> + AUDIO_LIBS> + fi
> AUDIO_TYPE=oss
> fi
>
see above
> @@ -328,6 +332,147 @@ fi
> AC_DEFINE_UNQUOTED(PACKAGE_PREFIX, "$PACKAGE_PREFIX", [Define
the package prefix])
> AC_SUBST(PACKAGE_PREFIX)
>
> +dnl #######################
> +dnl # C99 related tests #
> +dnl #######################
> +
<snip>> +AC_C99_NAN
> +AC_C99_ISFINITE
> +AC_C99_INFINITY
> +AC_C99_FP_INFINITE
> +AC_C99_FP_NAN
> +AC_C99_FP_ZERO
> +
> dnl #########################
> dnl # Make the output files #
> dnl #########################
>
That's the part I like least about this patch. Here's the problems:
1) Swfdec requires C99, or at least parts of it, and I don't really
want to change that requirement. (It uses -std=gnu99 after all).
2) It clutters configure.ac a lot. I prefer configure.ac files that
look halfway understandable.
3) It looks to me like this is OpenBSD specific.
I would argue that it's best to keep the C99 patches local to the
OpenBSD packages until your libc implements C99.
> diff --git a/libswfdec-gtk/swfdec_gtk_widget.c
b/libswfdec-gtk/swfdec_gtk_widget.c
> index 4836aa0..d71654f 100644
> --- a/libswfdec-gtk/swfdec_gtk_widget.c
> +++ b/libswfdec-gtk/swfdec_gtk_widget.c
> @@ -619,6 +620,7 @@ void
> swfdec_gtk_widget_set_renderer (SwfdecGtkWidget *widget,
cairo_surface_type_t renderer)
> {
> g_return_if_fail (SWFDEC_IS_GTK_WIDGET (widget));
> + renderer = FALSE;
>
> widget->priv->renderer = renderer;
> if (widget->priv->renderer_set == FALSE) {
>
That's probably a leftover thing from you trying to track down an X
bug. Others have reported a similar bug, see for example
> diff --git a/libswfdec-gtk/swfdec_playback_oss.c
b/libswfdec-gtk/swfdec_playback_oss.c
> index 9c94252..8d2e9d8 100644
> --- a/libswfdec-gtk/swfdec_playback_oss.c
> +++ b/libswfdec-gtk/swfdec_playback_oss.c
> @@ -22,7 +22,12 @@
> #include "config.h"
> #endif
>
> +#ifdef __OpenBSD__
> +#include <soundcard.h>
> +#else
> #include <sys/soundcard.h>
> +#endif
> +
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdlib.h>
> @@ -115,12 +120,19 @@ swfdec_stream_open (SwfdecPlayback *sound,
SwfdecAudio *audio)
> guint rate;
> int dsp_fd, ret, format, channels, fragsize;
>
> +#ifdef __OpenBSD__
> + dsp_fd = open("/dev/audio", O_WRONLY);
> + if (dsp_fd == -1) {
> + g_printerr ("Failed to open /dev/audio\n");
> + return;
> + }
> +#else
> dsp_fd = open("/dev/dsp", O_WRONLY);
> if (dsp_fd == -1) {
> g_printerr ("Failed to open /dev/dsp\n");
> return;
> }
> -
> +#endif
> format = AFMT_S16_LE;
> ret = ioctl(dsp_fd, SNDCTL_DSP_SETFMT, &format);
> if (ret == -1) {
>
The OSS patches you gave me are #if __OpenBSD__ - is there a way to
get them in a more friendly way? There has to be a portable way for
detecting OSS related issues? So either you keep those patches local
to OpenBSD or you clean them up. (Copying them from GStreamer's OSS
stuff for example).
It might be of interest to you that I intend to switch Swfdec's
default audio backend from ALSA to Pulse Audio and ideally get rid of
the other backends. No clue when that will happen, but it
will.> diff --git a/libswfdec/swfdec_as_interpret.c
b/libswfdec/swfdec_as_interpret.c
> index 639018c..235cff1 100644
> --- a/libswfdec/swfdec_as_interpret.c
> +++ b/libswfdec/swfdec_as_interpret.c
> @@ -46,7 +46,7 @@
> #include "swfdec_resource.h"
> #include "swfdec_resource_request.h"
> #include "swfdec_text_field_movie.h" // for typeof
> -
> +#include "stdlib.h"
> /* Define this to get SWFDEC_WARN'd about missing properties of
objects.
> * This can be useful to find out about unimplemented native properties,
> * but usually just causes a lot of spam. */
> diff --git a/libswfdec/swfdec_as_number.c b/libswfdec/swfdec_as_number.c
> index 4387301..396d226 100644
> --- a/libswfdec/swfdec_as_number.c
> +++ b/libswfdec/swfdec_as_number.c
> @@ -22,6 +22,7 @@
> #endif
>
> #include <math.h>
> +#include <stdlib.h>
>
> #include "swfdec_as_number.h"
> #include "swfdec_as_context.h"
Are those required for any functions used here, or is that a result of
the C99 fixes?
> diff --git a/libswfdec/swfdec_as_date.c b/libswfdec/swfdec_as_date.c
> index 148e61d..a8c921c 100644
> --- a/libswfdec/swfdec_as_date.c
> +++ b/libswfdec/swfdec_as_date.c
> @@ -904,7 +904,7 @@ swfdec_as_date_UTC (SwfdecAsContext *cx, SwfdecAsObject
*object, guint argc,
> SwfdecAsValue *argv, SwfdecAsValue *ret)
> {
> guint i;
> - int year, num;
> + int year=NULL, num;
> double d;
> BrokenTime brokentime;
>
I've cleaned the code there up a bit and that should hopefully fix
complaints about that variable being uninitialized. If not, please
complain.
> diff --git a/libswfdec/swfdec_as_object.c b/libswfdec/swfdec_as_object.c
<snip>> diff --git a/libswfdec/swfdec_as_types.c b/libswfdec/swfdec_as_types.c
<snip>> diff --git a/libswfdec/swfdec_bits.c b/libswfdec/swfdec_bits.c
<snip>> diff --git a/libswfdec/swfdec_codec_gst.c b/libswfdec/swfdec_codec_gst.c
<snip>>
applied, either directly or slightly modified. Please complain if they
still cause issues.
Cheers,
Benjamin