[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