Please add the correct copyright information at the top of all files. Also, would it make sense to add these to the existing bayer element and have the other formats selected by caps?

In caps, my bayer2rgb take the same sink video/x-raw-bayer and src video/x-raw-rgb format that the one in gstreamer. I will add more details about caps soon. I do this plugin because if i use bayer2rgb after jp462bayer the video raw is black and white instead of color. With bayer2rgb2 the video raw is in color. Moreover i have not written the code in bayer, so i can't explain the difference between them and i add multi-option for the output raw.

Created attachment 152210 [details] [review]
bayer2rgb2 and jp462bayer

I create a other patch with the modification about copyright

@sebastian, since the original bayer2rgb plugin did not rely at all on an external implementation (which is the case here with libdc1394), the plugin would have had an unclean architecture.

Regarding the caps, the problem is that there is no official bayer formats naming scheme, using properties proved easier to implement (and doesn't require touching gstreamer base types).

Comment on attachment 152210 [details] [review]
bayer2rgb2 and jp462bayer

Please use caps instead of properties for this, just extend the bayer caps to include the pattern.

Also, please add this as new elements to the existing bayer plugin if possible. And maybe replace the existing bayer2rgb should be replaced by this one instead of adding a new one.

Fundamental difference between bayer2rgb and bayer2rgb2 is that bayer2rgb implements (one of) the debayering algorithms, whereas bayer2rgb2 uses libdc1394's multiple methods as library.

Best thing imo would be to bundle bayer2rgb's algorithm (if different/better than libdc1394's) as a separate project/library, and add it as method to bayer2rgb.

Regarding proper bayer caps handling, this has been fixed recently (please see http://code.google.com/p/gst-plugins-elphel/source/browse/trunk/bayer2rgb2/src/gstbayer2rgb2.c#101)

What do you think ?

Sorry, i meant 
  * bundling bayer2rgb's demosaic method as external/standalone project (if better/different than libdc1394's)
  * integrating it to bayer2rgb2, or replacing bayer2rgb

Integrate it in bayer2rgb2 and replace bayer2rgb. And the caps look fine, the same was done in the "old" elements in gst-plugins-bad.

I would prefer that bayer2rgb be left as it is, and create a new element (in ext/) named dc1394bayer2rgb.  libdc1394 is yet another library to build in an embedded environment, when bayer2rgb may be sufficient for the needs of some people.  However, please add a note to the bayer2rgb documentation pointing to better quality demosaicing in dc1394bayer2rgb.

Also, please visually check all the demosaicing methods in libdc1394raw and disable the ones that are complete garbage (i.e., nearest).  With a comment in the source, of course.

(In reply to comment #10)
> I would prefer that bayer2rgb be left as it is, and create a new element (in
> ext/) named dc1394bayer2rgb.  libdc1394 is yet another library to build

The dc1394 conversion code is included in the plugin, there's no dependency on libdc1394

@David: "nearest" method is not complete "garbage", it shows images ;) I agree this isn't the most sophisticated one, but at least it's very light and fast, which is useful for very large images preview live generation. I agree non-working methods should be commented though.

Regarding edgesense, there apparently is a patent issue (for the U.S. at least), should we still include it in the upcoming patch ? Currently we added a ./configure option to enable it (--enable-edgesense), but disabled by default.

So now that we have integrated the original bayer2rgb's method, we still lack a name for it. If the method really is an implementation of http://www.siliconimaging.com/Specifications/AN3%20-%20Bayer%20Color%20Processing.PDF, maybe we should name it "Correlation-adjusted Linear", or "Siliconimaging" ???

Last, regarding the name of the plugin, we would like to keep bayer2rgb2. That way the name is simple and self-explanatory, contains "bayer" (->> gst-inspect | grep bayer). However, if you think/know that there are debayering algorithms which would create e.g. YUV data directly and susceptible to be integrated later on, maybe the plugin should rather be named "debayer".

If it's not ext/, then this should just be a patch to bayer2rgb.

The current method is bilinear.

There is no reason to have anything worse in quality than bilinear, since bilinear is as fast as memcpy.

I just realized that I never pushed my Orc conversion of bayer2rgb.  Not going to do it now, either, since it doesn't work anymore.

David, it looks like libdc1394's bilinear implementation is faster. I doubt keeping the 2 bilinear methods is worth it... Your orc version could be faster indeed. Why doesn't it work anymore ? Can you provide the Orc version so that we try to integrate it ?

Ideally, we should keep only one (the fastest).

We'll do performance tests to compare all the methods; if indeed the simple and nearest are as cpu-intensive as the bilinear, we shall remove them.

Yes, confirming that libdc1394's bilinear interpolation is faster (20 fps VS 15 fps for FullHD on Core 2 Quad 2.6 Ghz -- 32 bits).

Are you ok in us not keeping methods under "bilinear" and not porting bayer2rgb ?

Pushed my initial rewrite of the bilinear processing, before I did the orc conversion.  This works, and is about 50% faster.  I'm redoing the Orc stuff now.

Woohoo!  The orc code is doing 120+ fps for 1920x1080 on a similar cpu.  Probably will be a lot slower on 32-bit, as it uses a lot of pointer registers.  Will push once I get more formats implemented.

commit db7fe611edcaf03ac9d86d3fb0d96c28c4859fd7
Author: David Schleef <ds@schleef.org>
Date:   Mon May 30 23:43:39 2011 -0700

    bayer2rgb: Convert to Orc
    
    Seriously faster.  Algorithm is nearly the same as bilinear, which
    given the speed of this code, should be considered the baseline of
    quality.  Speed appears to be limited by memory bandwidth, so I
    didn't bother trying to make it any faster.


It would be a good idea to merge this with YCbCr matrixing and subsampling, which may mean it should move to the colorspace element.  It would definitely be a good idea to merge in JP46 handling, as the bayer interleaving is expensive to deal with twice.

Hi David,

120 fps, nice :)

Okay, so here's what we'll do now: finish bayer2rgb2 using the orced bilinear implementation as minimum/default method.

As for your suggestion of integrating JP46 handling, there are 2 features which make me think it's not a good idea:
  * first, it can resize (to multiples of the original resolution fast resizing like for jpeg by using binning, /2 or /4)
  * second, it's multi-threaded

I think it would be messy to integrate these features into colorspace. It's really quite dedicated to Elphel cameras.

As for integrating "bayer2rgb2" into colorspace, imo it would not make sense to implement multiple debayering methods into colorspace, because it would mean adding an API for choosing the method (which would make it less than generic).

Integrating only your orced bilinear method into colorspace would be nice, to handle the video/x-raw-bayer input, but we'll leave this to you.

This is a new patch that contains an update of bayer2rgb (proposed as replacement of the former bayer2rgb) and adds the new jp462bayer plugin.

[DUE TO ATTACHED FILE SIZE LIMITATIONS, PATCH CAN BE DOWNLOADED FROM: http://gst-plugins-elphel.googlecode.com/files/0001-Update-bayer2rgb-and-create-new-plugin-jp462bayer.patch]

For bayer2rgb :
- Adds a "method" property, referring to the debayering algorithm choice.
  By default, it uses David's ORCed bilinear algorithm. Other methods are from an inclusion of libdc1394's debayering library. For patent license issues (at least in the U.S.), the "EdgeSense" method has been removed but can still be obtained from the upstream project (http://code.google.com/p/gst-plugins-elphel/); simpler methods than bilinear have also been removed
- Adds the "bpp" property:
  This property allows to choose the raw bayer input format (8 bits or 16 bits)

For JP462bayer :
This algorithm allows to decode elphel camera raw streams (JP46 mode).
- "threads" property (0: auto, 1-4) : allows to decode elphel camera in multithreading (nb: incompatible with fast rescaling feature, see next item)
- By using specified output caps (containing a subset of the original resolution, devided by 4 or 16), the original JP46 stream will be downscaled without decoding ("fast resizing"), which is useful for low-quality live previews. This is currently single-threaded, therefore conflicting with the "threads" property; if combined with e.g. threads=2, a GST_ERROR is emitted

Created attachment 189606 [details] [review]
update bayer2rgb and new plugin jp462bayer

THE PREVIOUS MESSAGE IS OBSELETE, PLEASE DO NOT MIND
---------------------------------

This is a new patch that contains an update of bayer2rgb (proposed as
replacement of the former bayer2rgb) and adds the new jp462bayer plugin.

For bayer2rgb :
- Adds a "method" property, referring to the debayering algorithm choice.
  By default, it uses David's ORCed bilinear algorithm. Other methods are from
an inclusion of libdc1394's debayering library. For patent license issues (at
least in the U.S.), the "EdgeSense" method has been removed but can still be
obtained from the upstream project
(http://code.google.com/p/gst-plugins-elphel/); simpler methods than bilinear
have also been removed
- Adds the "bpp" property:
  This property allows to choose the raw bayer input format (8 bits or 16 bits)

For JP462bayer :
This algorithm allows to decode elphel camera raw streams (JP46 mode).
- "threads" property (0: auto, 1-4) : allows to decode elphel camera in
multithreading (nb: incompatible with fast rescaling feature, see next item)
- By using specified output caps (containing a subset of the original
resolution, devided by 4 or 16), the original JP46 stream will be downscaled
without decoding ("fast resizing"), which is useful for low-quality live
previews. This is currently single-threaded, therefore conflicting with the
"threads" property; if combined with e.g. threads=2, a GST_ERROR is emitted

Please set the correct author/email in git.

  http://wiki.sourcemage.org/Git_Guide#Any_other_initial_setup_I_need.3F

Please remove all the whitespace changes in configure.ac.

Method is spelled without a final e.

bpp should be in the caps, not as a property.  It also appears that there needs to be an endianness field in the caps for 16-bit bayer.

gstdc1394bayer.c needs a copyright statement.  This should be identical to the original.

Do not put incorrect copyright blocks on files (gstbayer2rgb.h, possibly others).  Ever.  This is especially bad because you changed both the copyright holder and the license.

The jp462bayer element should be part of the bayer plugin and directory.  It will make code sharing easier later.

The commit message should have more information about the changes you made.

The number of CPUs detection in jp462bayer.c will not compile on non-Linux platforms, nor will the pthread code.  You can either convert this to glib threads, or just make the code single-threaded.

Other minor things that should be fixed, but I'm not going to complain too loudly:

 - The enum names (BILINEAR, HQLINEAR) should have GST_BAYER2RGB_ prefixes.

 - We prefer guint8 over uint8_t.  (I realize the existing code doesn't do this.)

 - The colorspace element now supports 16-bit component RGB, which could be used instead of clipping to 8-bit.

Would be nice too see some progress on this patch, as there are some differences between the current bayer2rgb plugin and the results of libdc1394 bayer algorithm.

I'm sorry not to have responded rather. Unfortunately since I posted the patch, I had not time to update it. I'll see if I can update it for this summer.

Created attachment 244579 [details] [review]
cleaned up patch for 1.0

I've cleaned up and ported Anthony's patch to 1.0, but just for bayer2rgb. I think I addressed most of David's concerns. It's not quite complete, as a I have to test 16-bit, the downsample is all screwed up, and three of the other methods result in a black border. I'll try to finish these things up next week, but in the meantime any comments would be appreciated. Also note that before discovering this bug, I created a bug regarding 16-bit bayer caps:

https://bugzilla.gnome.org/show_bug.cgi?id=693666

thanks you, for your help.

Why is this still a separate element and not just different methods for the old one?

(In reply to comment #27)
> Why is this still a separate element and not just different methods for the old
> one?

I'm confused about your question, it is just adding new methods to the existing element, bayer2rgb, it is _not_ creating a new bayer2rgb2 element.

(In reply to comment #27)
> Why is this still a separate element and not just different methods for the old
> one?

I'm confused about your question, the patch I just attached is just adding new methods to the existing element, bayer2rgb, it is _not_ creating a new bayer2rgb2 element. If I could change the title of this bug I would, but I'm not the creator of it.

Created attachment 244855 [details] [review]
updated patch for 1.0 bayer plugin, adding 16-bit support

This patch implements 16-bit support, using format="bggr16, rggb16", etc. for bayer, and ARGB64 for RGB. I've added support for these to rgb2bayer as well for testing purposes.

I'd like some feedback on how I handled transform_caps for both elements; it seems very ugly, but I wasn't sure how to make it simpler.

Row stride alignment for Bayer doesn't seem to be well defined. videotestsrc definitely aligns to 4 bytes, but the Bayer elements don't make this assumption. If you can let me know what it should be, I'll make sure alignment is handled correctly.

Downsample either needs to be eliminated, to actually adjust caps to be half the width/height, or to upsample. I don't like that last option however.

Still the question about why having this as a separate element instead of merging it into the current element and just adding a method property to select the algorithm.

As the stride with bayer seems to be undefined in general, make a good choice for a default that is used often... and then add a GstMeta similar to GstVideoMeta to define additional information about the format... like the stride.

(In reply to comment #31)
> Still the question about why having this as a separate element instead of
> merging it into the current element and just adding a method property to select
> the algorithm.

I suggest you take a closer look at the patch; the confusion is probably a result of bad file naming. gstdc1394bayer.c just contains a slightly modified version of bayer.c from the libdc1394 project, it does not contain another element. Perhaps the gst prefix should be removed to avoid this confusion, especially since it doesn't depend on any GStreamer library.

> As the stride with bayer seems to be undefined in general, make a good choice
> for a default that is used often... and then add a GstMeta similar to
> GstVideoMeta to define additional information about the format... like the
> stride.

In my experience with framegrabbers, they don't bother to align strides at all, however I'll actually most often capture the video as GRAY16 and then use capssetter, so perhaps we should stick with the 4 byte alignment (this is actually not a problem with 16-bit bayer, assuming there's never a half-pattern).

FYI to people other than Sebastian, we were talking here about resurrecting depth and/or bpp fields for video/x-bayer instead of using formats like bggr16, in order to support the quite common 10- or 12-bits in 16-bit formats:
https://bugzilla.gnome.org/show_bug.cgi?id=693666

(In reply to comment #32)
> (In reply to comment #31)
> > Still the question about why having this as a separate element instead of
> > merging it into the current element and just adding a method property to select
> > the algorithm.
> 
> I suggest you take a closer look at the patch; the confusion is probably a
> result of bad file naming. gstdc1394bayer.c just contains a slightly modified
> version of bayer.c from the libdc1394 project, it does not contain another
> element. Perhaps the gst prefix should be removed to avoid this confusion,
> especially since it doesn't depend on any GStreamer library.

Ah sorry, nevermind then :)

> > As the stride with bayer seems to be undefined in general, make a good choice
> > for a default that is used often... and then add a GstMeta similar to
> > GstVideoMeta to define additional information about the format... like the
> > stride.
> 
> In my experience with framegrabbers, they don't bother to align strides at all,
> however I'll actually most often capture the video as GRAY16 and then use
> capssetter, so perhaps we should stick with the 4 byte alignment (this is
> actually not a problem with 16-bit bayer, assuming there's never a
> half-pattern).

So most hardware is generating bayer with no aligned strides? In that case we should do that by default too I guess, not useful to work against what most hardware already does.

Created attachment 245085 [details] [review]
updated patch to unify stride handling and support bpp field, cleanup and comments

Here's the latest patch. I've discovered that some framegrabbers do alignment, but there's nothing near a standard. One does no alignment, another on 4-bytes, another on 16-bytes, and others are configurable. Current patch assumes no alignment.

This patch also includes support for a bpp field, which works well with an Active Silicon Phoenix element and Basler acA2040 BG12 camera I have.

I can't think of anything else to change or improve, but let me know.

David, what do you think about this?

Nearly approved, pending resolution on caps:

+    GST_STATIC_CAPS ("video/x-bayer,format=(string){bggr,grbg,gbrg,rggb},"
+        "width=(int)[1,MAX],height=(int)[1,MAX],framerate=(fraction)[0/1,MAX];"
+        "video/x-bayer,format=(string){bggr16,grbg16,gbrg16,rggb16},"
+        "bpp=(int){10,12,14,16},endianness={1234,4321},width=(int)[1,MAX],"
+        "height=(int)[1,MAX],framerate=(fraction)[0/1,MAX]")

In the time since this patch was first created, we switched to using format to indicate endianness in audio/x-raw and video/x-raw.  I think should be changed to match.

In addition, I'd like to see the following things cleaned up in a follow up
patch:

+  /* half-pattern is not allowed */
+  g_assert (bayer2rgb->width % 2 == 0);

This should be a negotiation failure, not assertion.

+#define DEFAULT_MODE GST_BAYER2RGB_MODE_BILINEAR

(nitpick) This is called "method" in other places.

(In reply to comment #36)
> Nearly approved, pending resolution on caps:
> 
> +    GST_STATIC_CAPS ("video/x-bayer,format=(string){bggr,grbg,gbrg,rggb},"
> +       
> "width=(int)[1,MAX],height=(int)[1,MAX],framerate=(fraction)[0/1,MAX];"
> +        "video/x-bayer,format=(string){bggr16,grbg16,gbrg16,rggb16},"
> +        "bpp=(int){10,12,14,16},endianness={1234,4321},width=(int)[1,MAX],"
> +        "height=(int)[1,MAX],framerate=(fraction)[0/1,MAX]")
> 
> In the time since this patch was first created, we switched to using format to
> indicate endianness in audio/x-raw and video/x-raw.  I think should be changed
> to match.

ack

(In reply to comment #36)
> Nearly approved, pending resolution on caps:
> 
> +    GST_STATIC_CAPS ("video/x-bayer,format=(string){bggr,grbg,gbrg,rggb},"
> +       
> "width=(int)[1,MAX],height=(int)[1,MAX],framerate=(fraction)[0/1,MAX];"
> +        "video/x-bayer,format=(string){bggr16,grbg16,gbrg16,rggb16},"
> +        "bpp=(int){10,12,14,16},endianness={1234,4321},width=(int)[1,MAX],"
> +        "height=(int)[1,MAX],framerate=(fraction)[0/1,MAX]")
> 
> In the time since this patch was first created, we switched to using format to
> indicate endianness in audio/x-raw and video/x-raw.  I think should be changed
> to match.

Do you want to keep Bayer in video/x-bayer, or put it in video/x-raw? I suppose the latter would suggest adding it to gstvideo, but it probably wouldn't fit well within that model.

To indicate endianness, we'd have format={BGGR16_BE, BGGR16_LE, GRBG16_BE, ...}, using upper case to match video/x-raw format, or all lower case to be consistent with existing video/x-bayer formats...
 
> In addition, I'd like to see the following things cleaned up in a follow up
> patch:
> [...]

Will do.

(In reply to comment #38)
> (In reply to comment #36)
> > Nearly approved, pending resolution on caps:
> > 
> > +    GST_STATIC_CAPS ("video/x-bayer,format=(string){bggr,grbg,gbrg,rggb},"
> > +       
> > "width=(int)[1,MAX],height=(int)[1,MAX],framerate=(fraction)[0/1,MAX];"
> > +        "video/x-bayer,format=(string){bggr16,grbg16,gbrg16,rggb16},"
> > +        "bpp=(int){10,12,14,16},endianness={1234,4321},width=(int)[1,MAX],"
> > +        "height=(int)[1,MAX],framerate=(fraction)[0/1,MAX]")
> > 
> > In the time since this patch was first created, we switched to using format to
> > indicate endianness in audio/x-raw and video/x-raw.  I think should be changed
> > to match.
> 
> Do you want to keep Bayer in video/x-bayer, or put it in video/x-raw? I suppose
> the latter would suggest adding it to gstvideo, but it probably wouldn't fit
> well within that model.
> 
> To indicate endianness, we'd have format={BGGR16_BE, BGGR16_LE, GRBG16_BE,
> ...}, using upper case to match video/x-raw format, or all lower case to be
> consistent with existing video/x-bayer formats...

video/x-bayer and I guess lower-case for consistency with the existing bayer formats... unfortunately we can't change them all to upper-case now

-- 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/15.