[Bf-committers] Sequencer filter stack patch

[Xavier Thomas]

> just had a look at the patch. First: looks great!
>
> thanks


> But (small but :) ): you should change some things:
>
> The input_prefilter function was called prefilter, since it did some
> things, you can only do on the raw input data.
>

In the code it was mentioned that this function (input_preprocess) was for
doing some things that can only be done on the input image at his original
size (before it was scaled to project resolution) and it definitely operates
on RGB color space.


> That includes:
>
> * deinterlacing (that has to be done *before* the conversion from
>   YUV -> RGB takes place and was therefore handled by the imbuf-reader.
>   That is a must, since IMB_filtery just throws away one
>   field instead of doing correct field interpolation. Which is actually
>   impossible, since the underlying YUV-data is gone at that moment... )
>

I notice that in input_preprocess() the deinterlace filter (IMB_filtery) is
only used when the imbuf reader can't handle it. I planed to keep the actual
deinterlace flag but put it in the input panel only for sequence type that
support it. My deinterlace filter will only replace the IMB_filtery
"fallback".

* color conversion could be done a lot more efficient and correctly
>   if done directly on input data. (read: YUV!)
>

Some things could be more efficient in the YUV->RGB conversion it should be
usable on the RGB input also, I suppose that is why everything (except
deinterlace) is done after the conversion. I really did not plan to make the
preprocess operation working in the 2 color space (as this was never the
case). This could be the subject of another proposal.

* flip-Y could be done very efficiently within the input reader, too as
>   part of the color conversion step YUV -> RGB.
>

Could be done efficiently by  inverting the scale factor also (if some
transformation are used)


So: you may want to
> a) take deinterlacing out of your filter stack (leave that to the image
>    reader like it was implemented before)
>

I will not remove the imbuf reader deinterlace option for suported format
that is for sure.


> b) fold all color changing "point functions" (operate one pixel at a time)
>    into a single function like you did with the transformation matrix.
>    (maybe using lookup tables for byte input?)
>

In fact some "color filter" can also be done by matrix manipulation:
(r,g,b)= matrix[3][3] * (r1,g1,b1)

I thinked of adding this type of filter but prefered to stick with something
basic to start with. But there is nothing complicated here.
I could include some brightness, contrast (black point, withe point), hue
and saturation adjustement, but not gamma.

The problem with this is that it cannot stack well with other type (as they
are applied togheter) but they can be resricted to be on the top of the
stack/applied first). (that make me think that transform operation
could/should be restricted to be on the bottom (applied last).


>    After that, we could take a look, how that can be folded into
>    the image reader. (optional, but definitely worth the change for
>    better quality)
>

Is really the imbuf reader the best place for this? I recall that those
filters can be applied to scene strips and meta strips.


>    To clarify:
>    now we do:      YUV -> RGB -> color change
>    We *should* do: YUV -> color change -> RGB
>

I understand, but I really thing this king of refactor could be made in a
second step. A option would be to add an option to the imbuf reader to do
byte->float convertion in the YUV->RGB conversion so it is not lossy
anymore.


xavier