Re: [PATCH v3] media: Add video processing entity functions

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Friday 25 Mar 2016 16:14:59 Hans Verkuil wrote:
> On 03/25/2016 03:19 PM, Laurent Pinchart wrote:
> > Add composer, format converter and scaler functions, as well as generic
> > video processing to be used when no other processing function is
> > applicable.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  Documentation/DocBook/media/v4l/media-types.xml | 34 ++++++++++++++++++++
> >  include/uapi/linux/media.h                      |  8 ++++++
> >  2 files changed, 42 insertions(+)
> > 
> > Changes since v2:
> > 
> > - Fix typo (any other mean -> any other means)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/media-types.xml
> > b/Documentation/DocBook/media/v4l/media-types.xml index
> > 5e3f20fdcf17..38e8d6c25d49 100644
> > --- a/Documentation/DocBook/media/v4l/media-types.xml
> > +++ b/Documentation/DocBook/media/v4l/media-types.xml
> > @@ -121,6 +121,40 @@
> >  	    <entry><constant>MEDIA_ENT_F_AUDIO_MIXER</constant></entry>
> >  	    <entry>Audio Mixer Function Entity.</entry>
> >  	  </row>
> > +	  <row>
> > +	    
<entry><constant>MEDIA_ENT_F_PROC_VIDEO_GENERIC</constant></entry>
> > +	    <entry>Generic video processing, when no other processing 
function
> > +		   is applicable.
> > +	    </entry>
> 
> I'll be honest and say that I don't really like this. A VIDEO_GENERIC
> function is only marginally more useful than UNKNOWN. And I think I prefer
> UNKNOWN for now until we have a clear picture how these functions are going
> to work. The main missing piece in this puzzle are properties that allow us
> to register multiple functions and some decision as to what the scope of
> 'functions' is going to be.
> 
> You mentioned that you have a few entities that are using this function, but
> if you would specify the exact function of those entities, what would the
> function name(s) be?

I'm not sure, otherwise I would have proposed more precise functions :-)

Two of the entities just apply look up tables. They can thus be used for 
various purpose, such as gamma correction, A-law/µ-law (de)compression, ...

The last entity is an interface between the VSP and the display device and 
just handles buffering, clock domain crossing and synchronization.

> > +	  <row>
> > +	    
<entry><constant>MEDIA_ENT_F_PROC_VIDEO_COMPOSER</constant></entry>
> > +	    <entry>Video composer (blender). An entity capable of video
> > +		   composing must have at least two sink pads and one source
> > +		   pad, and composes input video frames onto output video
> > +		   frames. Composition can be performed using alpha blending,
> > +		   color keying, raster operations (ROP), stitching or any other
> > +		   means.
> > +	    </entry>
> > +	  </row>
> 
> This one looks OK to me.
> 
> > +	  </row>
> > +	    
<entry><constant>MEDIA_ENT_F_PROC_VIDEO_CONVERTER</constant></entry>
> > +	    <entry>Video format converter. An entity capable of video format
> > +		   conversion must have at least one sink pad and one source
> > +		   pad, and convert the format of pixels received on its sink
> > +		   pad(s) to a different format output on its source pad(s).
> > +	    </entry>
> > +	  </row>
> 
> This is too vague as well, I think. You said that you don't consider
> de-interlacing a converter function, but what about colorimetry conversion?
> Debayer? 4:2:2 to 4:2:0 conversion or vice versa?

I'd consider that as video format conversion, yes.

The three entities that implement this function in the vsp1 driver are 
ARGB8888 <-> AHSV8888 converters and RGB <-> YUV converters (with various RGB 
and YUV formats supported).

> > +	  <row>
> > +	    <entry><constant>MEDIA_ENT_F_PROC_VIDEO_SCALER</constant></entry>
> > +	    <entry>Video scaler. An entity capable of video scaling must have
> > +		   at least one sink pad and one source pad, and scaling the
> > +		   video frame(s) received on its sink pad(s) to a different
> > +		   resolution output on its source pad(s). The range of
> > +		   supported scaling ratios is entity-specific and can differ
> > +		   between the horizontal and vertical directions. In particular
> > +		   scaling can be supported in one direction only.
> > +	    </entry>
> > +	  </row>
> 
> This looks OK too, although would sensor binning and/or skipping also be
> considered scaling?

I would consider them as scaling, yes. Sakari, any opinion on that ?


> >  	</tbody>
> >        </tgroup>
> >      </table>

-- 
Regards,

Laurent Pinchart