I'd suggest #1, since that's the way we've gone for video/x-raw and audio/x-raw in 1.0.

I'm not familiar with 12 bit formats to know how they're packed.  Each packing format would need a different format string.

It would be a good time to review the other fields, too.  I imagine there needs to be a source transfer function (gamma) and/or dynamic range compression (a-law, etc).

I have a wishlist goal to write an entire set of imaging pipeline elements, but that seems really adventurous for the amount of free time I have.

Just found the below bug which sort of depends on this one since the patch there can do 16-bit Bayer:
https://bugzilla.gnome.org/show_bug.cgi?id=607342

Most Bayer cameras that are greater than 8-bit are actually 10- or 12-bits, just stored in 16-bits. In these cases the new bayer2rgb would produce very dark images. I've gone with the #1 approach above of using format=rggb16, but we either need a way of indicating the actual bits used or another element that will do the conversion before bayer2rgb. In 0.10 I might have suggested using bpp and depth as part of the caps, but that's out for 1.0. Would a separate element then be best?

I've written such an element, videolevels, for going from GRAY16 to GRAY8, but ideally the demosaicing would be performed before going to 8-bits, so perhaps I could extend my element to go from ARGB64 (the only 16-bit RGB format I can find in GstVideo) to one of the RGB32 formats. Anyone interested in this?

Something like gbrg12_16 could be used, similar to S24_32 for audio for example.

Something like gbrg12_16 could be used, similar to S24_32 for audio for example.

I suppose that could work, though that would entail adding *10_16, *12_16, *14_16, and *16 for each of the four patterns, or 16 new formats. That might be too much bloat for too little benefit, considering that no one else is interested in these formats right now. I think the separate conversion element element can work for now, but if performance considerations become important for some, then perhaps we could implement them.

As an aside, the same issue applies to GRAY16 formats, most I've seen would actually be GRAY12_16 or GRAY14_16.

In that case a depth field in the caps makes sense, yes. And not having it there would mean full depth (8 or 16 or 24 or 32 or whatever we come up with).

So which of the following should we do for 12-bit in 16-bit:
video/x-bayer,format={bggr,rggb,gbrg,grbg},depth=16,bpp=12
video/x-bayer,format={bggr16,rggb16,gbrg16,grbg16},bpp=12

The first is a bit more elegant, but also isn't entirely backwards compatible, so the second would be better, where the default bpp=16. I'll change my bayer2rgb patch at #607342 to support this second form of caps unless you disagree.

I think the second one would be best, yes.

It's been a while, but I've been using the caps as you suggested. Here they are minus width/height/framerate:

video/x-bayer
      format: { bggr16, grbg16, gbrg16, rggb16 }
         bpp: {10, 12, 14, 16}
  endianness: {1234, 4321}

Another party has showed interest, so before we close this ticket I'll solicit their feedback.

Use "le" / "be" suffixes instead of another field, like for the other video formats :)

I suppose 1.0 did drop the use of the endianness field in favor of GRAY16_LE/GRAY16_BE, and audio went to formats like S16LE. Note the varying inclusion of underscore. So consistency with lower-case use of existing 8-bit Bayer formats, and underscore use of existing video formats, do we arrive at:

bggr16_le, grbg16_le, gbrg16_le, rggb16_le, bggr16_be, grbg16_be, gbrg16_be, rggb16_be

Sorry for being verbose :)

And I just now notice that some formats like I420_10BE don't use an underscore before "BE"/"LE". Hrm.

Emmanuel Pacaud (Aravis) and Edgar Thier (Imaging Source) have indicated via email they have no opinion on this matter, other than that they want it to be standardized.

Given that the more recent video formats have no underscore, I'd say we go with the following:

bggr16le, grbg16le, gbrg16le, rggb16le, bggr16be, grbg16be, gbrg16be, rggb16be

+1

Another question would be how packed bayer formats should be described.

Cameras can send not only 'regular' bayer but also packed data where data for 2 pixel is stuffed into one byte. This allows The application to extract 8-bit bayer by simply ignoring certain bytes.
I know of three variations of this. Packed, SPacked and MIPI Packed.

The question is how these transport formats should be described so that everyone is on the same page when it comes to unpacking.
A new field in the GstStructure, Maybe? 

The description in the fourcc headers of The Imaging Source would be as follows. [] indicating a single byte:

#define FOURCC_BGGR12		    mmioFOURCC('B', 'G', '1', '2') /* 12  BGBG.. GRGR.. */
#define FOURCC_GBRG12		    mmioFOURCC('G', 'B', '1', '2') /* 12  GBGB.. RGRG.. */
#define FOURCC_GRBG12		    mmioFOURCC('B', 'A', '1', '2') /* 12  GRGR.. BGBG.. */
#define FOURCC_RGGB12		    mmioFOURCC('R', 'G', '1', '2') /* 12  RGRG.. GBGB.. */

#define FOURCC_GRBG12_SPACKED   mmioFOURCC('G', 'R', 'C', 'p') /* 12  u8, [pix0_lo][pix0_hi | pix1_hi][pix1_lo] */
#define FOURCC_RGGB12_SPACKED   mmioFOURCC('R', 'G', 'C', 'p') /* 12   */
#define FOURCC_GBRG12_SPACKED   mmioFOURCC('G', 'B', 'C', 'p') /* 12   */
#define FOURCC_BGGR12_SPACKED   mmioFOURCC('B', 'G', 'C', 'p') /* 12   */

#define FOURCC_GRBG12_PACKED    mmioFOURCC('G', 'R', 'C', 'P') /* 12  u8, [pix0_hi][pix0_lo | pix1_lo][pix1_hi] */
#define FOURCC_RGGB12_PACKED    mmioFOURCC('R', 'G', 'C', 'P') /* 12   */
#define FOURCC_GBRG12_PACKED    mmioFOURCC('G', 'B', 'C', 'P') /* 12   */
#define FOURCC_BGGR12_PACKED    mmioFOURCC('B', 'G', 'C', 'P') /* 12   */

#define FOURCC_GRBG12_MIPI_PACKED       mmioFOURCC('G', 'R', 'D', 'P') /* 12  u8, [pix0_hi][pix1_hi][pix0_lo | pix1_lo] */
#define FOURCC_RGGB12_MIPI_PACKED       mmioFOURCC('R', 'G', 'D', 'P') /* 12   */
#define FOURCC_GBRG12_MIPI_PACKED       mmioFOURCC('G', 'B', 'D', 'P') /* 12   */
#define FOURCC_BGGR12_MIPI_PACKED       mmioFOURCC('B', 'G', 'D', 'P') /* 12   */

Looking at V4L, they specify eight Bayer formats [1], one of which is just our original video/x-bayer, and three of which would correspond to the bggr16le, etc format. Two formats use compression, A-LAW or DPCM. The remaining two formats are for 10- and 12-bit packed, which I believe correspond to MIPI Packed.

I've not had much use of packed formats, and when I do the first thing I usually do is to unpack it for processing and even storage.

I suppose you could define formats like bggr_p/bggr_sp/bggr_mipi, along with the bpp field, but these packed formats aren't something I'm very familiar with so I'd defer to you or someone else.

[1]: https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-rgb.html

How about the following list:

video/x-bayer,format={bggr16,rggb16,gbrg16,grbg16},bpp={10,12,16}
video/x-bayer,format={gbrg10s,grbg10s,rggb10s,bggr10s,gbrg10sp,grbg10sp,rggb10sp,bggr10sp,gbrg10m,grbg10m,rggb10m,bggr10m}  # bpp=10 is implied
video/x-bayer,format={gbrg12s,grbg12s,rggb12s,bggr12s,gbrg12sp,grbg12sp,rggb12sp,bggr12sp,gbrg12m,grbg12m,rggb12m,bggr12m}  # bpp=12 is implied
video/x-bayer,format={gbrg,grbg,rggb,bggr} # bpp=8 is implied

The v4l format SBGGR12P would equal bggr12p.
This way the packed formats are explicitly different from the unpacked formats.
I have not yet encountered a case where endianess had to be considered, so I do not know if that has to be considered.

At the moment, I'm leaning towards thinking we should be deprecating the video/x-bayer approach, and rolling a swathe of bayer variants into video/x-raw and supporting them in libgstvideo.

That's really the only way I can see to get them into glupload for GPU debayering.

There's a couple of things about that approach that give me pause though.
1) There's so many Bayer variants, they'll quickly become the largest part of the GstVideoFormat list for something that's not really used very much
2) It's a bit hard to implement the generic pack and unpack functions for libgstvideo without implicitly just implementing simple debayering in software.

(In reply to Jan Schmidt from comment #18)

Any reason to not just add them into glupload "manually" outside of the GstVideo code? Yes it's a bit of a special codepath, but the bayer stuff is only used one way and the only thing we ever do with it is to convert it to regular formats, so no need to pollute everything with them?

The amount of code required to wedge it into glupload looked pretty bad at the time because the GL stack depends on GstVideoInfo heavily.

Locally, I have WIP patches that add 8-bit and 16-bit bayer formats to the GstVideoInfo set and do debayering in a GL element. That was a lot easier, but it does 'pollute' the format set. The other benefit of that path is that the frames then support GstVideoMeta for stride and mapping info easily.

Either way is a bit awkward.

Wouldn't it be possible to have a GstBufferPool that 'allocates' buffers in GPU memory? Unless I am missing something that way the src would simply fill the buffer, the gpu could do debayering and the rest can be left untouched.

(In reply to Edgar Thier from comment #21)
> Wouldn't it be possible to have a GstBufferPool that 'allocates' buffers in
> GPU memory? Unless I am missing something that way the src would simply fill
> the buffer, the gpu could do debayering and the rest can be left untouched.

GPU memory is not always mappable on host CPU. Though, if your source is v4l2, then you can request DMABuf, which we support in the gl elements.

(In reply to Jan Schmidt from comment #20)
> The amount of code required to wedge it into glupload looked pretty bad at
> the time because the GL stack depends on GstVideoInfo heavily.
> 
> Locally, I have WIP patches that add 8-bit and 16-bit bayer formats to the
> GstVideoInfo set and do debayering in a GL element. That was a lot easier,
> but it does 'pollute' the format set. The other benefit of that path is that
> the frames then support GstVideoMeta for stride and mapping info easily.
> 
> Either way is a bit awkward.

My impression is that GstVideoInfo approach is less work overall. For the format pollution, it's the documentation that should be improved in my opinion. We should try and find a way to create format sections to help browse the formats. Long term, it would even be nice to have per format (or group of formats) documentation, to remove the confusion, a bit like linux-media doc does. gtkdoc might be the bottleneck here.

For the pack/unpack, I'm wondering if there is a clean way to just not implement it. We still want debayering to be done by GL or specific debayering element I believe ? and do we care about bayering really ? What I mean, is that I'm not sure it's useful to fit bayer format into videoconvert, specially that a debayer element would have extra API for passing matrix (iirc gamma / color balance, I'm not expert here).

I think it's fine to put it into GstVideoFormat & co - there isn't really anything rawer than bayer, is there?

"Just" have to figure out how to make it work best with our existing API.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/86.