[libv4l] : Introduce define for lookup table size

  Make lookup table size configurable at compile-time.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>
  

On 09/05/17 13:04, Pavel Machek wrote:
> 
> Make lookup table size configurable at compile-time.

I don't think I'll take this patch. The problem is that if we really add
support for 10 or 12 bit lookup tables in the future, then just changing
LSIZE isn't enough.

This patch doesn't really add anything as it stands.

Regards,

	Hans

>     
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/lib/libv4lconvert/processing/libv4lprocessing-priv.h b/lib/libv4lconvert/processing/libv4lprocessing-priv.h
> index e4a29dd..55e1687 100644
> --- a/lib/libv4lconvert/processing/libv4lprocessing-priv.h
> +++ b/lib/libv4lconvert/processing/libv4lprocessing-priv.h
> @@ -25,6 +25,8 @@
>  #include "../libv4lsyscall-priv.h"
>  
>  #define V4L2PROCESSING_UPDATE_RATE 10
> +/* Size of lookup tables */
> +#define LSIZE 256
>  
>  struct v4lprocessing_data {
>  	struct v4lcontrol_data *control;
> @@ -32,15 +34,15 @@ struct v4lprocessing_data {
>  	int do_process;
>  	int controls_changed;
>  	/* True if any of the lookup tables does not contain
> -	   linear 0-255 */
> +	   linear 0-LSIZE-1 */
>  	int lookup_table_active;
>  	/* Counts the number of processed frames until a
>  	   V4L2PROCESSING_UPDATE_RATE overflow happens */
>  	int lookup_table_update_counter;
>  	/* RGB/BGR lookup tables */
> -	unsigned char comp1[256];
> -	unsigned char green[256];
> -	unsigned char comp2[256];
> +	unsigned char comp1[LSIZE];
> +	unsigned char green[LSIZE];
> +	unsigned char comp2[LSIZE];
>  	/* Filter private data for filters which need it */
>  	/* whitebalance.c data */
>  	int green_avg;
> @@ -48,7 +50,7 @@ struct v4lprocessing_data {
>  	int comp2_avg;
>  	/* gamma.c data */
>  	int last_gamma;
> -	unsigned char gamma_table[256];
> +	unsigned char gamma_table[LSIZE];
>  	/* autogain.c data */
>  	int last_gain_correction;
>  };
> diff --git a/lib/libv4lconvert/processing/libv4lprocessing.c b/lib/libv4lconvert/processing/libv4lprocessing.c
> index b061f50..6d0ad20 100644
> --- a/lib/libv4lconvert/processing/libv4lprocessing.c
> +++ b/lib/libv4lconvert/processing/libv4lprocessing.c
> @@ -74,7 +74,7 @@ static void v4lprocessing_update_lookup_tables(struct v4lprocessing_data *data,
>  {
>  	int i;
>  
> -	for (i = 0; i < 256; i++) {
> +	for (i = 0; i < LSIZE; i++) {
>  		data->comp1[i] = i;
>  		data->green[i] = i;
>  		data->comp2[i] = i;
> diff --git a/lib/libv4lconvert/processing/whitebalance.c b/lib/libv4lconvert/processing/whitebalance.c
> index c74069a..2dd33c1 100644
> --- a/lib/libv4lconvert/processing/whitebalance.c
> +++ b/lib/libv4lconvert/processing/whitebalance.c
> @@ -27,7 +27,7 @@
>  #include "libv4lprocessing-priv.h"
>  #include "../libv4lconvert-priv.h" /* for PIX_FMT defines */
>  
> -#define CLIP256(color) (((color) > 0xff) ? 0xff : (((color) < 0) ? 0 : (color)))
> +#define CLIPLSIZE(color) (((color) > LSIZE) ? LSIZE : (((color) < 0) ? 0 : (color)))
>  #define CLIP(color, min, max) (((color) > (max)) ? (max) : (((color) < (min)) ? (min) : (color)))
>  
>  static int whitebalance_active(struct v4lprocessing_data *data)
> @@ -111,10 +111,10 @@ static int whitebalance_calculate_lookup_tables_generic(
>  
>  	avg_avg = (data->green_avg + data->comp1_avg + data->comp2_avg) / 3;
>  
> -	for (i = 0; i < 256; i++) {
> -		data->comp1[i] = CLIP256(data->comp1[i] * avg_avg / data->comp1_avg);
> -		data->green[i] = CLIP256(data->green[i] * avg_avg / data->green_avg);
> -		data->comp2[i] = CLIP256(data->comp2[i] * avg_avg / data->comp2_avg);
> +	for (i = 0; i < LSIZE; i++) {
> +		data->comp1[i] = CLIPLSIZE(data->comp1[i] * avg_avg / data->comp1_avg);
> +		data->green[i] = CLIPLSIZE(data->green[i] * avg_avg / data->green_avg);
> +		data->comp2[i] = CLIPLSIZE(data->comp2[i] * avg_avg / data->comp2_avg);
>  	}
>  
>  	return 1;
>
  

Hi!

> > Make lookup table size configurable at compile-time.
> 
> I don't think I'll take this patch. The problem is that if we really add
> support for 10 or 12 bit lookup tables in the future, then just changing
> LSIZE isn't enough.
> 
> This patch doesn't really add anything as it stands.

Well, currently we have 256, 255 and 0xff sprinkled through the code,
when it means to say "lookup table size". That is quite wrong (because
you can't really grep "what depends on the table size).

And BTW with the LSIZE set to 1024, 10 bit processing seems to
work. So it is already useful, at least for me.

But now I noticed the patch is subtly wrong:

> > -#define CLIP256(color) (((color) > 0xff) ? 0xff : (((color) < 0) ? 0 : (color)))
> > +#define CLIPLSIZE(color) (((color) > LSIZE) ? LSIZE : (((color) <
0) ? 0 : (color)))

This should be LSIZE-1.

So I need to adjust the patch. But I'd still like you to take (fixed
version) for documentation purposes...

Best regards,
									Pavel

> >  #define CLIP(color, min, max) (((color) > (max)) ? (max) : (((color) < (min)) ? (min) : (color)))
> >  
> >  static int whitebalance_active(struct v4lprocessing_data *data)
> > @@ -111,10 +111,10 @@ static int whitebalance_calculate_lookup_tables_generic(
> >  
> >  	avg_avg = (data->green_avg + data->comp1_avg + data->comp2_avg) / 3;
> >  
> > -	for (i = 0; i < 256; i++) {
> > -		data->comp1[i] = CLIP256(data->comp1[i] * avg_avg / data->comp1_avg);
> > -		data->green[i] = CLIP256(data->green[i] * avg_avg / data->green_avg);
> > -		data->comp2[i] = CLIP256(data->comp2[i] * avg_avg / data->comp2_avg);
> > +	for (i = 0; i < LSIZE; i++) {
> > +		data->comp1[i] = CLIPLSIZE(data->comp1[i] * avg_avg / data->comp1_avg);
> > +		data->green[i] = CLIPLSIZE(data->green[i] * avg_avg / data->green_avg);
> > +		data->comp2[i] = CLIPLSIZE(data->comp2[i] * avg_avg / data->comp2_avg);
> >  	}
> >  
> >  	return 1;
> >
  

On 16/05/17 14:45, Pavel Machek wrote:
> Hi!
> 
>>> Make lookup table size configurable at compile-time.
>>
>> I don't think I'll take this patch. The problem is that if we really add
>> support for 10 or 12 bit lookup tables in the future, then just changing
>> LSIZE isn't enough.
>>
>> This patch doesn't really add anything as it stands.
> 
> Well, currently we have 256, 255 and 0xff sprinkled through the code,
> when it means to say "lookup table size". That is quite wrong (because
> you can't really grep "what depends on the table size).
> 
> And BTW with the LSIZE set to 1024, 10 bit processing seems to
> work. So it is already useful, at least for me.
> 
> But now I noticed the patch is subtly wrong:
> 
>>> -#define CLIP256(color) (((color) > 0xff) ? 0xff : (((color) < 0) ? 0 : (color)))
>>> +#define CLIPLSIZE(color) (((color) > LSIZE) ? LSIZE : (((color) <
> 0) ? 0 : (color)))
> 
> This should be LSIZE-1.
> 
> So I need to adjust the patch. But I'd still like you to take (fixed
> version) for documentation purposes...

I much rather do this as part of a longer series that actually adds 10 bit support.

The problem is that adding support for 10 bit doesn't mean you can just use LSIZE
all the time since you still need support for 8 bit as well.

E.g. CLIPLSIZE makes no sense, I would expect to see a CLIP256 and a CLIP1024.

So it becomes a bit more complex than just adding an LSIZE define.

Regards,

	Hans

> 
> Best regards,
> 									Pavel
> 
>>>  #define CLIP(color, min, max) (((color) > (max)) ? (max) : (((color) < (min)) ? (min) : (color)))
>>>  
>>>  static int whitebalance_active(struct v4lprocessing_data *data)
>>> @@ -111,10 +111,10 @@ static int whitebalance_calculate_lookup_tables_generic(
>>>  
>>>  	avg_avg = (data->green_avg + data->comp1_avg + data->comp2_avg) / 3;
>>>  
>>> -	for (i = 0; i < 256; i++) {
>>> -		data->comp1[i] = CLIP256(data->comp1[i] * avg_avg / data->comp1_avg);
>>> -		data->green[i] = CLIP256(data->green[i] * avg_avg / data->green_avg);
>>> -		data->comp2[i] = CLIP256(data->comp2[i] * avg_avg / data->comp2_avg);
>>> +	for (i = 0; i < LSIZE; i++) {
>>> +		data->comp1[i] = CLIPLSIZE(data->comp1[i] * avg_avg / data->comp1_avg);
>>> +		data->green[i] = CLIPLSIZE(data->green[i] * avg_avg / data->green_avg);
>>> +		data->comp2[i] = CLIPLSIZE(data->comp2[i] * avg_avg / data->comp2_avg);
>>>  	}
>>>  
>>>  	return 1;
>>>
>
  

Hi!

> >>> Make lookup table size configurable at compile-time.
> >>
> >> I don't think I'll take this patch. The problem is that if we really add
> >> support for 10 or 12 bit lookup tables in the future, then just changing
> >> LSIZE isn't enough.
> >>
> >> This patch doesn't really add anything as it stands.
> > 
> > Well, currently we have 256, 255 and 0xff sprinkled through the code,
> > when it means to say "lookup table size". That is quite wrong (because
> > you can't really grep "what depends on the table size).
> > 
> > And BTW with the LSIZE set to 1024, 10 bit processing seems to
> > work. So it is already useful, at least for me.
> > 
> > But now I noticed the patch is subtly wrong:
> > 
> >>> -#define CLIP256(color) (((color) > 0xff) ? 0xff : (((color) < 0) ? 0 : (color)))
> >>> +#define CLIPLSIZE(color) (((color) > LSIZE) ? LSIZE : (((color) <
> > 0) ? 0 : (color)))
> > 
> > This should be LSIZE-1.
> > 
> > So I need to adjust the patch. But I'd still like you to take (fixed
> > version) for documentation purposes...
> 
> I much rather do this as part of a longer series that actually adds 10 bit support.
> 
> The problem is that adding support for 10 bit doesn't mean you can just use LSIZE
> all the time since you still need support for 8 bit as well.
> 
> E.g. CLIPLSIZE makes no sense, I would expect to see a CLIP256 and a CLIP1024.
> 
> So it becomes a bit more complex than just adding an LSIZE define.

Yes, proper 10 bit support will be more complex (and I'm not sure if
I'll be able to do it, I might need help there). OTOH... table size is
used at 7 places in three different files, and 256 is _very_ common
constant.

So IMO this makes sense regardless of full 10-bit support.

Best regards,
									Pavel


> >>>  #define CLIP(color, min, max) (((color) > (max)) ? (max) : (((color) < (min)) ? (min) : (color)))
> >>>  
> >>>  static int whitebalance_active(struct v4lprocessing_data *data)
> >>> @@ -111,10 +111,10 @@ static int whitebalance_calculate_lookup_tables_generic(
> >>>  
> >>>  	avg_avg = (data->green_avg + data->comp1_avg + data->comp2_avg) / 3;
> >>>  
> >>> -	for (i = 0; i < 256; i++) {
> >>> -		data->comp1[i] = CLIP256(data->comp1[i] * avg_avg / data->comp1_avg);
> >>> -		data->green[i] = CLIP256(data->green[i] * avg_avg / data->green_avg);
> >>> -		data->comp2[i] = CLIP256(data->comp2[i] * avg_avg / data->comp2_avg);
> >>> +	for (i = 0; i < LSIZE; i++) {
> >>> +		data->comp1[i] = CLIPLSIZE(data->comp1[i] * avg_avg / data->comp1_avg);
> >>> +		data->green[i] = CLIPLSIZE(data->green[i] * avg_avg / data->green_avg);
> >>> +		data->comp2[i] = CLIPLSIZE(data->comp2[i] * avg_avg / data->comp2_avg);
> >>>  	}
> >>>  
> >>>  	return 1;
> >>>
> >
  

Hi!

> I much rather do this as part of a longer series that actually adds 10 bit support.

> The problem is that adding support for 10 bit doesn't mean you can just use LSIZE
> all the time since you still need support for 8 bit as well.

Heh. I was afraid to hear that. I can probably support 10-bits for
white balance somehow.

How would complete support for 10-bits work? Introduce RGB48 and
modify processing to work in 16-bits?

And the big question: is it possible to do processing with
libv4lconvert, without actually have v4l device available?

For example... I do have a simple camera application. In a viewfinder
mode, it has to work at GRBG10 format, because any conversion is just
too slow. We'd also want to save raw GRBG10 image to disk for raw
processing. So far so good. But we'd also want to save jpeg, which
means converting existing buffer to RGB24, applying white balance (and
maybe bad-pixel, lens shading, vignetting compensation) and saving
jpeg.

I'm trying to use this for conversion:

static void convert_rgb(struct dev_info *dev, struct v4l2_format sfmt, void *buf, struct v4l2_f\
ormat dfmt, void *buf2, int wb)
{
        struct v4lconvert_data *data = v4lconvert_create(dev->fd);
        int res;

        printf("Converting first.");
        if (wb) {
                struct v4l2_control ctrl;
                ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE;
                ctrl.value = 1;
		v4lconvert_vidioc_s_ctrl(data, &ctrl);
	}
        res = v4lconvert_convert(data, &sfmt, &dfmt, buf, SIZE, buf2, SIZE);
	printf("Converting: %d\n", res);
        v4lconvert_destroy(data);
}

but

1) it feels like improper use of internal functions.

2) it crashes when I attempt to do white balance processing.

Is there an interface I could use? Should I create interface for
v4lconvert_ for this kind of processing? Any preferences how it should
look like?

Best regards,
									Pavel