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

Larry Robinson silver-dad
Mon Jan 10 06:20:21 CET 2011


On 1/9/2011 2:30 PM, Michael Niedermayer wrote:
> 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
>>>
Maybe I could just add them to the arg string passed to the "buffer" 
filter init function.  Then there's no change to AVFilterGraph, which 
addresses the next comment.
>>>> 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.
>
>
Its a simple project to get my feet wet as I learn the architecture of 
ffmpeg so I've done it.  The attached patch and new file implement a 
move of the colorspace enums into libavcore/colorspace.h   I tested it 
on my system (x86_64-w64-mingw32) and it builds and passes all tests 
without error.
>> 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
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: avcodec_with_colorspace_in_avcore.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110109/1c19d3f4/attachment.asc>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: colorspace.h
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110109/1c19d3f4/attachment.txt>



More information about the ffmpeg-devel mailing list