[FFmpeg-devel] [PATCH] Re: How can I determine color_range from a filter?
Michael Niedermayer
michaelni
Sun Jan 9 23:30:59 CET 2011
On Sun, Jan 09, 2011 at 08:26:25PM +0100, Stefano Sabatini wrote:
> 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
you have a point of course, still
1. its largely stylistic nitpickery IMHO
2. we will have filters that will use libavcodec stuff like (i)dct / wavelets
for postprocessing and noise reduction
These filters will need libavcodec headers for compilation, at runtime
dlopen can be used to avoid a dependancy.
Thus IMHO we have thousands of things that are more important than moving these
enums around but iam not against them being moved if someone wants to do it.
> compile/install libavfilter without installing libavcodec (configure
> --enable-libavfilter --disable-libavcodec),
no problem here
> in a distro you could have
> libavfilter-dev and yet not libavcodec-dev.
it wouldnt work, we dont have 4 independant configure scripts, and
quadruplicating the build system across 5 split -dev packages seems like a
bad idea
and if you meant the distro simply physically removd libavcodec.
We should nozt support this castration and if someone still wanted he can
leave the header there as well if he already manually cuts things randomly
away
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/275eb00f/attachment.pgp>
More information about the ffmpeg-devel
mailing list