I don't get how endianness applies to these formats. Can someone elaborate, please?

you have 16bits for 565, so on big endian it looks like:
rrrrrggg gggbbbbb
On Little endian, it is:
gggbbbbb rrrrrggg

So the general approach is to stuff the value into a guint16 using shifts and then TO_BE/LE to get the endianness right, kinda like this:
guint16 pixel;
pixel = (r << 11) | ((g & 0x3f) << 5) | (b & 0x1f);
pixel = GUINT16_TO_BE(pixel);

That is not the approach used in ffmpegcolorspace. For RGB555, it does:

((uint16_t *)(d))[0] = ((r >> 3) << 10) | ((g >> 3) << 5) | (b >> 3) | ((a << 8) & 0x8000);

For RGB565, it does:

((uint16_t *)(d))[0] = ((r >> 3) << 11) | ((g >> 2) << 5) | (b >> 3);

Should this be fixed to use something such as this GUINT16_TO_BE?

ffmpegcolorspace assumes native-endianness for 15 and 16 bit video, which is technically wrong, but unlikely to ever be noticed.  The reason is because 15/16 bit video is getting to be very rare, big endian is getting to be rare, and transmitting uncompressed RGB video over the network (such that endianness might be a problem) is pretty rare.  The combination of all three is *very* rare.

ffmpegcolorspace assumes native-endianness for 15 and 16 bit video, which is technically wrong, but unlikely to ever be noticed.  The reason is because 15/16 bit video is getting to be very rare, big endian is getting to be rare, and transmitting uncompressed RGB video over the network (such that endianness might be a problem) is pretty rare.  The combination of all three is *very* rare.

So the attached patches are correct and ffmpegcolorspace needs some fixing? That's what I thought too

Martin, are you going to fix ffmpegcolorspace to support both endianness correctly?

Does it maybe make sense to always have byte-order==BIG_ENDIAN as before and only adjust the masks? Depends on what ffmpegcolorspace had done in the past though, that should be kept.

The reason I did the things like I did what because all the RGB/BGR 24/32 format caps are defined using endianness=BIG_ENDIAN, but the RGB 15/16 formats use endianness=BYTE_ORDER in current video.h.

So if the proper way to make this is to stick to BIG_ENDIAN like the other formats (althought this might be kind of weird because as David said earlier, big endian is getting to be rare), then the masks would also have to be adjusted to be expressed in big endian, instead of native endianness as they are now.

I could obviously do pretty much the same task using little endianness, which might make for "practical" sense, althought the other RGB format caps are defined with big endianness format.

The task to make the format respect one endianness is obviously pretty much the same, I just wonder weither it's better to use big endian (like other RGB formats) or little endian (that might be a more probable possibility).

It's not really possible to keep byte_order=BIG_ENDIAN as green_mask = 0xC003, BIG_ENDIAN is not the same as green_mask = 0x3C0, LITTLE_ENDIAN.

Why do you say that it's not the same thing?  Endianness is the just the order of the bytes, so why couldn't the mask be expressed in both?

The endianness macros would only be used to convert between known (hard-coded BIG_ENDIAN or LITTLE_ENDIAN) endianness and host endianness.

Why wouldn't it be possible to keep byte_order = BIG_ENDIAN?

(In reply to comment #10)
> Why do you say that it's not the same thing?

You are confusing "what something means when I assume definitions for words" and "what something means according to the definitions in the documentation".

Masks are defined to be contiguous sets of bits.  Changing that definition would be an ABI break.

Also, supporting wrong-endian 15/16-bit video has a very low usefulness/developer_time quotient, imo.

Ok, I guess that's why currently, RGB15/16 are the only caps defined in video.h with BYTE_ORDER (host) endianness, because it's the only format that has a mask that crosses the byte boundary?

So what would be the proper way to deal with the masks in local endianness, but to store the resulting 16 bits in a way that can be set across network?

After a little bit more research, here are my thoughts.

The simplest option would be to leave RGB15/16 as it is right now, which is with host (or BYTE_ORDER) endianness.  If that option is chosen, it means that the patch can be accepted, because it doesn't modify the caps or the interpretation of the data.  It just adds new GstVideoFormat enums and adds the missing support for the parsing of those already existing caps.

The endianness topic discussed here, althought relevant, is imho a little bit out of the scope of the proposed fix.  Adding proper endianness support could be another (different) feature request that would involve more work:

- New pixel formats might be defined, such as {RGB,BGR}_{15,16}_{LE,BE}
- New routines would be added to ffmpegcolorspace
- Existing plugins would have to be modified (for instance, videoscale assumes host endianness for RGB565 format, because that's the way it is right now)
- Etc.

So if the __current__ behavior regarding RGB565 and RGB555 formats is acceptable, I suggest incorporating the patch and leaving the more robust endianness handling to another request (which, I agree with David, would be very low priority).
Otherwise, it means that to get this patch in, I will implement complete and correct support for those formats right away.

I tend to lean towards the first option.  Any thoughts?

commit b8f330dea63861a5cfce6f2e15e7d716a7d8bc43
Author: Martin Bisson <martin.bisson@gmail.com>
Date:   Tue Jun 1 16:45:34 2010 +0000

    video.{c,h}: Fix an endianness bug fix.
    
    This commit makes sure the endianness is ok for RGB/BGR 15/16 formats.

commit f5a690f860d9eba396e0a4e06cd07b4489fbd7e3
Author: Martin Bisson <martin.bisson@gmail.com>
Date:   Tue Jun 1 14:42:54 2010 +0000

    video.{c,h}: Add support for RGB and BGR with 15 and 16 bits.