Re: [patch, libv4l]: Introduce define for lookup table size

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 2926 bytes --]

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;
> >>>
> > 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]