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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110109/c2623fca/attachment.pgp>