[FFmpeg-devel] [PATCH] Re: How can I determine color_range from a filter?

[Stefano Sabatini]

On date Sunday 2011-01-09 19:42:27 +0100, Michael Niedermayer encoded:
> On Sun, Jan 09, 2011 at 12:11:02AM +0100, Stefano Sabatini wrote:
> > On date Friday 2011-01-07 11:12:59 -0800, Larry Robinson encoded:
> > > So here's the easy part, adding the new properties to AVFilterLink.
> > > Let me know if this is inconsistent with style, architecture, etc.
> > > 
> > > Now - how to fill them:
> > > It looks like configure_filters(...) in ffmpeg.c starts the filter
> > > chain on line 364 by calling avfilter_graph_create_filter(&ist...).
> > > I propose to do the following:
> > > 
> > > 1) Create a new data structure, located in cmdutils.h:
> > >     typedef struct AVVolatileVideoProperties
> > >     {
> > >         enum AVColorPrimaries color_primaries;
> > >         enum AVColorTransferCharacteristic color_trc;
> > >         enum AVColorSpace colorspace;
> > >         enum AVColorRange color_range;
> > >         AVChromaLocation chroma_sample_location;
> > >     } AVVolatileVideoProperties;
> > > 
> > > NOTE:
> > > If I do this, cmdutils.h will have to be included after avcodec.h,
> > > because that's where AVColorPrimaries, etc. are declared.
> > > The second option is to  #include avcodec.h  in cmdutils.h - this
> > > will prevent breaking code that includes cmdutils but not avcodec.
> > > The last option is to declare the structure in both ffmpeg.c and
> > > ffplay.c and keep it out of cmdutils.h altogether.
> > > 
> > > Is there any guidance to help me make the decision?
> > 
> > I can't see the problem, cmdutils.h already includes avcodec.h.
> >  
> > > 2) Add a new function:  set_volatile_video_properties(
> > > AVCodecContext * ctx )  to {cmdutils.h, cmdutils.c}
> > >     which will copy the values into the AVVolatileVideoProperties
> > > structure.
> > > 
> > > The issues and options noted in (1) apply here also.  Guidance would
> > > be appreciated.
> > > 
> > > 3) Add an additional parameter to avfilter_graph_create_filter, i.e.
> > > 
> > > int avfilter_graph_create_filter(AVFilterContext **filt_ctx, AVFilter *filt,
> > >                                  const char *name, const char *args,
> > > void *opaque,
> > >                                  AVFilterGraph *graph_ctx,
> > > AVVolatileVideoProperties *vvp);
> > > 
> > > If vvp is not NULL, the properties in vvp will be copied to the
> > > AVFilterLink.
> > > 
> > > 4) The new properties will need to be propagated along the filter
> > > chain (I'll figure out how to do this), and ultimately used by
> > > encoders (beyond my expertise).
> > 
> > Maybe a better solution would be to use the option (libavutil/opt.h)
> > system in AVFilterGraph.
> 
> iam not so sure if opt.h is a good idea
> 
> 
> > 
> > Also I'm not sure colorspace should be a global property of the filter
> > graph, rather than a property of the single link/filter.
> 
> yes, agree here, and they could even change when the input changes
> (thats more a architectural design note and not a request to actually support
> it at first)
> 
> 
> > 
> > > On 1/6/2011 6:28 PM, Michael Niedermayer wrote:
> > > >On Thu, Jan 06, 2011 at 04:06:33PM -0800, Larry Robinson wrote:
> > > >>Let me know what the (few others) are and I will try to add them :-) .
> > > >     /**
> > > >      * Chromaticity coordinates of the source primaries.
> > > >      * - encoding: Set by user
> > > >      * - decoding: Set by libavcodec
> > > >      */
> > > >     enum AVColorPrimaries color_primaries;
> > > >
> > > >     /**
> > > >      * Color Transfer Characteristic.
> > > >      * - encoding: Set by user
> > > >      * - decoding: Set by libavcodec
> > > >      */
> > > >     enum AVColorTransferCharacteristic color_trc;
> > > >
> > > >     /**
> > > >      * YUV colorspace type.
> > > >      * - encoding: Set by user
> > > >      * - decoding: Set by libavcodec
> > > >      */
> > > >     enum AVColorSpace colorspace;
> > > >
> > > >     /**
> > > >      * MPEG vs JPEG YUV range.
> > > >      * - encoding: Set by user
> > > >      * - decoding: Set by libavcodec
> > > >      */
> > > >     enum AVColorRange color_range;
> > > >
> > > >     /**
> > > >      * This defines the location of chroma samples.
> > > >      * - encoding: Set by user
> > > >      * - decoding: Set by libavcodec
> > > >      */
> > > >     enum AVChromaLocation chroma_sample_location;
> > [...]
> > 
> > > Index: libavfilter/avfilter.h
> > > ===================================================================
> > > --- libavfilter/avfilter.h	(revision 26253)
> > > +++ libavfilter/avfilter.h	(working copy)
> > > @@ -25,6 +25,7 @@
> > >  #include "libavutil/avutil.h"
> > >  #include "libavcore/avcore.h"
> > >  #include "libavcore/samplefmt.h"
> > > +#include "libavcodec/avcodec.h"
> > >  
> > >  #define LIBAVFILTER_VERSION_MAJOR  1
> > >  #define LIBAVFILTER_VERSION_MINOR 72
> > > @@ -614,6 +615,42 @@
> > >       * input link is assumed to be an unchangeable property.
> > >       */
> > >      AVRational time_base;
> > > +
> > > +    /**
> > > +     * Chromaticity coordinates of the source primaries.
> > > +     * - input link: Set by previous filter, read only
> > > +     * - output link: May be changed by filter
> > > +     */
> > > +    enum AVColorPrimaries color_primaries;
> > 
> > I want to avoid unconditional dependency of libavfilter on libavcodec.
> 
> using enums from avcodec.h does not create a runtime dependancy so i dont
> see a problem

I believe that assuming the existence of libavcodec headers when you
compile a libavfilter application is wrong, indeed you could even
compile/install libavfilter without installing libavcodec (configure
--enable-libavfilter --disable-libavcodec), in a distro you could have
libavfilter-dev and yet not libavcodec-dev.
-- 
FFmpeg = Fancy and Fast Magic Programmable Elaborated Gadget