Created attachment 16386 [details] [review]
proposed patch

I don't really see why we would want to use BIG_ENDIAN instead of
host-native endian order. I mean, basically, nothing changes in
handling anything, and we're just confusing ourselves, right? The only
advantage of your approach is that it makes it easier to understand
how the stuff is aligned in bytes, but normally, people shouldn't have
to care about that in the first place...

I'm really not trying to be a PITA, but I do prefer to use host-native
endian-order in the caps... Is there any specific case where your
approach makes an obvious difference to the current one and where it's
actually an obvious improvement?

Also, note that with your approach, casting a 32-bit RGB buffer to
guint32 and then applying the mask no longer works on little-endian
machines, for obvious reasons. This is a clear disadvantage, in my
opinion...

Could someone fill me in on how this works?

If I have a 1 pixel video buffer and I do "pixel = *((guint32) data)"
I assume I can do "redpart = pixel & gst_caps_get_int(caps,
"red_mask")" at any time with any RGB video on any machine.

Is this true or not before/after the patch?

You can still cast a 32-bit RGB pixel to a guint32 and pull out the
color component using the color masks -- but you need to convert the
color masks to the host's byte order first.

Right now, a lot of things are using endianness==G_BYTE_ORDER, which
means that for a given subpixel arrangement in memory, the masks are
different depending on the endianness.  I think that a particular
memory arrangement should have exactly one authoritative caps, and
using G_BYTE_ORDER doesn't allow this.  Currently, few plugins get the
color masks correct for G_BYTE_ORDER on big endian machines.  Thus,
one of the primary benefits of using G_BIG_ENDIAN is that caps can be
defined without #ifdefs.

(The main advantage of using G_BIG_ENDIAN over G_LITTLE_ENDIAN is that
the masks read in memory order.)

Example code:

  endianness = gst_caps_get_int(caps,"endianness");
  red_mask = gst_caps_get_int(caps,"red_mask");
  if(endianness!=G_BYTE_ORDER)red_mask=GUINT32_SWAP_BE_LE(red_mask);
  
  pixel = *(guint32 *)data;
  red_part = pixel&red_mask;

for bpp=16, you'd use

  pixel = *(guint16 *)data;
  red_part = pixel&red_mask;

This works for any combination of host endianness and data endianness.
 I don't think that anything less addresses the problem correctly.

However, I don't think that anyone will actually use caps in this way.
 The primary use of RGB caps appears to be converting to/from a
library's #define'd format.  This mostly just means knowing how to
specify the library's format as gst caps.

Benjamin, before the patch, it works that way. Don't forget you have
to bitshift to actually get the red value, so:

guint32 red = *(guint32 *) data & redmask;
int num_shifts = 0;
while (redmask && !(redmask & 1))
  redmask >>= 1;
red >>= num_shifts;

would give you the actual bytevalue of red.

Dave, you're saying a lot of plugins currently have to use #ifdefs to
convert masks depending on byte order. That sounds weird, it'd mean
that the libs we depend on don't actually make proper use of RGB data
depending on endianness, which seems more like an issue with them than
with us, unless this is for a specific purpose. I don't see this
happening a lot, actually.

24-bit and 32-bit RGB formats are typically byte-oriented, not
word-oriented.  There is no inherint endianness in the data as there
is with 16-bit or 15-bit, there is merely arrangement.

The reason there aren't many #ifdefs currently is because nothing
really works on big-endian machines.

> Benjamin, before the patch, it works that way.

Only on little endian machines.

Here's a concrete example of the problem.  Swfdec creates 24 bit RGB
images where the ((guint8 *)data)[0] is red, [1] is green, [2] is
blue, [3] is red, etc., independent of host endianness.  To describe
this using G_BYTE_ORDER, we need:

#ifdef G_BYTE_ORDER == G_BIG_ENDIAN
  c = GST_CAPS_NEW (
    "swfdec_videosrc",
    "video/raw",
      "format",         GST_PROPS_FOURCC (GST_MAKE_FOURCC
('R','G','B',' ')),
      "bpp",            GST_PROPS_INT (24),
      "depth",          GST_PROPS_INT (24),
      "endianness",     GST_PROPS_INT (G_BYTE_ORDER),
      "red_mask",       GST_PROPS_INT (0xff0000),
      "green_mask",     GST_PROPS_INT (0x00ff00),
      "blue_mask",      GST_PROPS_INT (0x0000ff)
  );
#else
  c = GST_CAPS_NEW (
    "swfdec_videosrc",
    "video/raw",
      "format",         GST_PROPS_FOURCC (GST_MAKE_FOURCC
('R','G','B',' ')),
      "bpp",            GST_PROPS_INT (24),
      "depth",          GST_PROPS_INT (24),
      "endianness",     GST_PROPS_INT (G_BYTE_ORDER),
      "red_mask",       GST_PROPS_INT (0x0000ff),
      "green_mask",     GST_PROPS_INT (0x00ff00),
      "blue_mask",      GST_PROPS_INT (0xff0000)
  );
#endif

Arguably, one can make this cleaner, although not smaller.  However,
one must use #ifdefs for GST_PAD_TEMPLATE_FACTORY().

But this will go for most file-source/file-sink elements. I'm assuming
that swf inherently always uses big-endian inside? Similarly, AVI
always contains little endian data. So RGB32 data in an AVI file would
always look like BGRA:

((guint8 *) data)[0] = b;
((guint8 *) data)[1] = g;
((guint8 *) data)[2] = r;
((guint8 *) data)[3] = a;

Most files/streams simply have an internally defined, fixed, byte
order. This is simply a portability feature of these files. So
file/stream elements, in the old case, would always need #ifdefs. In
the new case, they wouldn't.

Device elements, on the other hand, will always wan the native byte
order, e.g. for sending stuff to the sound card, xvideo,
reading/writing v4l, etc. This is because the byte order here always
depends on the host system. In the old case, we didn't need #ifdefs
all around, you simply describe (RGB32 as example)
red_mask=0x00ff0000, blue_mask=0x0000ff00, green_mask=0x000000ff and
endianness is G_BYTE_ORDER. In the new case, we get exactly the
problem that file elements have in the old case: we'll need #ifdefs,
and the caps you described above will be exactly what's needed here, now.

For encoders/decoders/filters, I'm not sure what applies most. This
basically depends on whether the underlying lib/backend reads data as
bytes or as guint32's. I'm guessing some libs do the one, other libs
do the other so I can't say much here.

So my conclusion is that for some elements, fixed byte order is easier
in describing their caps, while for others, native byte order is
easier. It doesn't make any difference, really...

I'll take this patch with me in the caps remake.

(I still don't like it, but that's beyond the point ;) )

*** This bug has been marked as a duplicate of 116387 ***