[FFmpeg-devel] [PATCH] lavfi: add lut3d filter.

[Clément Bœsch]

On Wed, May 22, 2013 at 10:06:56AM +0200, Stefano Sabatini wrote:
> On date Tuesday 2013-05-21 04:49:06 +0200, Clément Bœsch encoded:
[...]
> > + at itemize
> > + at item 3dl
> > + at item cube
> > + at item dat
> > + at item m3d
> > + at end itemize
> 
> Adding some notes about these types may help (software used to create
> them, reference, etc., so the user is not at the mercy of a search
> engine).
> 

I've added the software that seem related to those formats. Unfortunately,
I don't own any, I just had a few random samples, so I can't really check
anything. Also, some extensions are shared with multiple softwares, and of
course different layouts. That's hard to tell which one belongs to which
one. There is certainly a room for improvements here.

> Also I think having a tool for generating files in these formats
> (e.g. using an ff expression) would be useful. Attaching a sample file
> would also help to test it.
> 

Yes. But with the Hald CLUT we can do much better actually. See below.

Also, those sample are pretty huge (see
http://www.pandora-int.com/pandora/download/manual/cube_example.html)

[...]
> > +/**
> > + * @todo Hald CLUT?
> 
> missing @file notice.
> 

Added.

> "Hald CLUT" sounds like funny nonsense to me.
> 

It is described here: http://www.quelsolaar.com/technology/clut.html

I removed that chunk since I've implemented it in another filter already
(I'm still considering merging it with lut3d though).

The interesting thing about these Hald CLUT is that they are stored as
images, which allow much more awesome things. Typically, you generate an
identity LUT into a picture, process that picture with whatever color
filters, and then you can just use this picture to process your input. It
is interesting if your color filters are slow, or simply not implemented
in FFmpeg.

The other idea is to change this reference picture into a video stream to
allow updating it through the time. But we'll discuss all of this in the
patch I'll send soon.

[...]
> > +#define PREV(x) ((int) s->x     )
> > +#define NEXT(x) ((int)(s->x + 1))
> 
> I'm not fond of this kind of macros which assume the existence of a
> variable, please make s a formal parameter and/or move the macro
> within the function definition where it is used.
> 

Changed.

> > +
> > +static inline struct rgbvec interp_trilinear(const LUT3DContext *lut3d,
> 
> > +                                             const struct rgbvec *s)
> 
> "s" is a poor name
> 

It's handy in this situation. I've renamed it in the caller to
"scaled_rgb" so you can make sense out of it.

[...]
> 
> For non trivial math a reference to an article/book/resource where you
> find the formulas is always good for the reader.
> 

Added some info above the 3 interpolation functions.

[...]
> I can't spot apparent mistakes, very nice job.

Cool, then... pushed. Thanks

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130522/2c01f02e/attachment.asc>