[gst-devel] alignment issues

[Daniel Gazard]

"Ronald S. Bultje" <R.S.Bultje at students.uu.nl> writes:

Hi Ronald,

> On Tue, 2004-04-06 at 21:43, Daniel Gazard wrote: 
>> Here is patches (diff were made from the 0.8.0 version) that I use to fix
>> these alignment problems. You can find them at the following address:
>> http://www.epita.fr/~gazard_d/gst-patchs/
>
> I'll modify them slightly. The 8-bit reading functions (...) should be
> replaced by a single one (without LE/BE).

Yes it does not make sense at all to make several 8 bits reading
functions. I just did it because they are needed by the
READ_UINT_BITS_FUNCTION macro in gstasfdemux.c.

In effect , I replaced
-      *ret = GUINT ## bits ## _FROM_LE (*((guint ## bits *) data));
by
+      *ret = GST_READ_UINT ## bits ## _LE (data);

If there is only one macro 8 bit this may cause when « bits » is
replaced by 8 (this is the case, see READ_UINT_BITS_FUNCTION (8)).

> The functions in your "don't
> support unaligned mem access" seem odd to me, since you bitshift guchar
> and then cast the end result to the required type (e.g. guint64). As far
> as I know, this is not allowed: you should cast first, then bitshift
> (else, the bitshift will use a default 32bit length on popular archs,
> therefore overflowing and losing information). So I'd propose changing
> them to:
>
> # define GST_READ_UINT64_BE(data)      (((guint64) data[0]) << 56 | \
>                                         ((guint64) data[1]) << 48 | \
>                                         ((guint64) data[2]) << 40 | \
>                                         ((guint64) data[3]) << 32 | \
>                                         ((guint64) data[4]) << 24 | \
>                                         ((guint64) data[5]) << 16 | \
>                                         ((guint64) data[6]) <<  8 | \
>                                         ((guint64) data[7])        ))

You are absolutely right. However, I think it also be safer to cast
data in guchar* before, even if it should not be necessary (data
should already be a guchar* but if it's not it may cause other
alignment problems).

> Same for the others. I personally don't care much for the GST_PUCHAR()
> and GST_GUINT64() macros, is there any reason why it was in there? The
> above looks much more readable to me than your macros. ;).

Agreed, you can pull them out ; there are definitely useless (they
were where there to improve the code readability but obviously this is
not the case :-))

> Then, you seem to be doing something weird with GST_DISABLE_LOADSAFE in
> the udpsink/tcpsink elements, can you elaborate on that? It seems like
> you're disabling some _link() functions, which isn't a good idea.

Link function were almost already disabled by GST_DISABLE_LOADSAFE. I
just put unused variable (when GST_DISABLE_LOADSAFE is set) in the
ifndef to avoid compiler complains and try to fix a few building
problems. 

gstudpsink.c:
- I put udpsink, serv_addr, serverhost, FILE *f, guint bc_val in the
ifndef since there are not used when GST_DISABLE_LOADSAFE is defined.
- I put « g_free (buf) » in the ifndef too because buf is not declared
when GST_DISABLE_LOADSAFE is defined (which it yield a compilation
error).

gsttcpsink.c:
- Without the patch, when GST_DISABLE_LOADSAVE is defined the switch
statement in gst_tcpsink_sink_link is disabled but a few case (as the
C keyword sense) subsist (CONTROL_NONE and default) which cause a
build fail.

gsttcpsrc.c: 
- Disable ret, client_sock and caps since they are not used when
GST_DISABLE_LOADSAVE is defined.
- Put xmlDocPtr doc in the ifndef to disable the libxml dependency
(which is, as far as I know, what GST_DISABLE_LOADSAFE should do).

gstudpsrc.c
- Put « g_free (buf) » in the ifndef for the same reason as
gstudpsink.c.

I'm absolutely not sure if it is the right way to fix these problems
but it seems there are troubles with this files when
GST_DISABLE_LOADSAFE is defined. May be you should compile gstreamer
with GST_DISABLE_LOADSAFE set and then try to compile plugins to see
exactly what's wrong.

> Anyway, thanks for the patch. If you want, please provide a new patch.
> Else I'll queue this somewhere on bugzilla and apply myself as soon as I
> find some time to go over it and change the above.

Ok, I will try to provide another one as soon as possible.

Thanks,

-- Daniel