[FFmpeg-devel] [RFC] libavfilter audio framework - split patches

S.N. Hemanth Meenakshisundaram smeenaks
Mon Jul 12 01:35:48 CEST 2010


Stefano Sabatini wrote:
> On date Sunday 2010-07-11 16:47:21 +0200, Michael Niedermayer encoded:
>   
>> On Sun, Jul 11, 2010 at 06:58:15AM -0700, S.N. Hemanth Meenakshisundaram wrote:
>>     
>>> S.N. Hemanth Meenakshisundaram wrote:
>>>       
>>>> I am sending out the working lavfi audio patches in the order Stefano 
>>>> requested for easy committing.
>>>>         
>>> This has avfilter.c changes. Extra debug logs, nit fixes and fixed the code 
>>> for avfilter_get_samples_ref in case of insufficient permissions.
>>>       
>>>  avfilter.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 80 insertions(+)
>>> f7a14f4c3e438f73480a2d034f869a48d0554881  avfilter.c.diff
>>> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
>>> index 38ca3b1..6d265aa 100644
>>> --- a/libavfilter/avfilter.c
>>> +++ b/libavfilter/avfilter.c
>>> @@ -60,6 +60,22 @@ void avfilter_unref_pic(AVFilterPicRef *ref)
>>>      av_free(ref);
>>>  }
>>>  
>>> +AVFilterSamplesRef *avfilter_ref_samples(AVFilterSamplesRef *ref, int pmask)
>>> +{
>>> +    AVFilterSamplesRef *ret = av_malloc(sizeof(AVFilterSamplesRef));
>>> +    *ret = *ref;
>>> +    ret->perms &= pmask;
>>> +    ret->buffer->refcount++;
>>> +    return ret;
>>> +}
>>>       
>> duplicate of avfilter_ref_pic()
>>
>>
>>     
>>> +
>>> +void avfilter_unref_samples(AVFilterSamplesRef *ref)
>>> +{
>>> +    if (!(--ref->buffer->refcount))
>>> +        ref->buffer->free(ref->buffer);
>>> +    av_free(ref);
>>> +}
>>>       
>> duplicate of avfilter_unref_pic()
>>
>>
>>     
>>> +
>>>  void avfilter_insert_pad(unsigned idx, unsigned *count, size_t padidx_off,
>>>                           AVFilterPad **pads, AVFilterLink ***links,
>>>                           AVFilterPad *newpad)
>>> @@ -97,7 +113,9 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
>>>      link->dst     = dst;
>>>      link->srcpad  = srcpad;
>>>      link->dstpad  = dstpad;
>>> +    link->type    = src->output_pads[srcpad].type;
>>>      link->format  = PIX_FMT_NONE;
>>> +    link->aformat = SAMPLE_FMT_NONE;
>>>  
>>>      return 0;
>>>  }
>>> @@ -210,6 +228,20 @@ AVFilterPicRef *avfilter_get_video_buffer(AVFilterLink *link, int perms, int w,
>>>      return ret;
>>>  }
>>>  
>>> +AVFilterSamplesRef *avfilter_get_samples_ref(AVFilterLink *link, int perms, int size,
>>> +                                              int64_t channel_layout, enum SampleFormat sample_fmt, int planar)
>>> +{
>>> +    AVFilterSamplesRef *ret = NULL;
>>> +
>>> +    if (link_dpad(link).get_samples_ref)
>>> +        ret = link_dpad(link).get_samples_ref(link, perms, size, channel_layout, sample_fmt, planar);
>>> +
>>> +    if (!ret)
>>> +        ret = avfilter_default_get_samples_ref(link, perms, size, channel_layout, sample_fmt, planar);
>>> +
>>> +    return ret;
>>> +}
>>>       
>> duplicate of avfilter_get_video_buffer()
>>
>>
>>     
>>> +
>>>  int avfilter_request_frame(AVFilterLink *link)
>>>  {
>>>      DPRINTF_START(NULL, request_frame); dprintf_link(NULL, link, 1);
>>> @@ -221,6 +253,14 @@ int avfilter_request_frame(AVFilterLink *link)
>>>      else return -1;
>>>  }
>>>  
>>> +int avfilter_request_samples(AVFilterLink *link)
>>> +{
>>> +    if (link_spad(link).request_samples)
>>> +        return link_spad(link).request_samples(link);
>>> +    else if (link->src->inputs[0])
>>> +        return avfilter_request_samples(link->src->inputs[0]);
>>> +    else return AVERROR(EINVAL);
>>> +}
>>>  int avfilter_poll_frame(AVFilterLink *link)
>>>  {
>>>      int i, min=INT_MAX;
>>>       
>> duplicate avfilter_request_frame()
>>     
>
> OK I indeed have the same concerns. I see two ways for fixing this:
> implementing some form of heiry macros, *or* generalize the code.
>
> For example let's consider this:
>
> AVFilterSamplesRef *avfilter_ref_samples(AVFilterSamplesRef *ref, int pmask)
> {
>     AVFilterSamplesRef *ret = av_malloc(sizeof(AVFilterSamplesRef));
>     *ret = *ref;
>     ret->perms &= pmask;
>     ret->buffer->refcount++;
>     return ret;
> }
>
> AVFilterSamplesRef *avfilter_ref_pic(AVFilterPicRef *ref, int pmask)
> {
>     AVFilterPicRef *ret = av_malloc(sizeof(AVFilterPicRef));
>     *ret = *ref;
>     ret->perms &= pmask;
>     ret->buffer->refcount++;
>     return ret;
> }
>
> We may factorize function definitions like this:
> #define DEFINE_AVFILTER_REF(refname, refstruct) *avfilter_ref_##refname(refstruct *ref, int pmask) \
> { \
>     refstruct *ret = av_malloc(sizeof(refstruct)); \
>     *ret = *ref; \
>     ret->perms &= pmask; \
>     ret->buffer->refcount++; \
>     return ret; \
> }
>
> DEFINE_AVFILTER_REF(pic    , AVFilterPicRef    );
> DEFINE_AVFILTER_REF(samples, AVFilterSamplesRef);
>
> The alternative may be to define and use an AVFilterBuffer, define
> avfilter_ref_buffer()/avfilter_unref_buffer(), and make each
> AVFilterPicRef and AVFilterPicSamples refer to such an
> AVFilterBuffer.
>
> The latter would break ABI/API, but that's maybe currently not a
> concern.
>
> Regards.
>   
Hi,

I can do the 2nd option itself and make both AVFilterPicRef and 
AVFilterSamplesRef point to a common AVFilterBuffer. However, a couple 
of questions/observations about this:

1. Then every audio buffer would also carry around w, h and format 
(PixelFormat) fields. Is this ok?

2. From the code, it looks like w and h can be safely removed from the 
buffer struct since it is never used in any of the filters. All filters 
use only the w and h values in the referencing PicRef struct. I would 
only need to remove the code in defaults.c (get_video_buffer) that 
assigns the w and h values of the buffer struct. Can I do this (as a 
separate patch of course)?

3. The PixelFormat field however would need to be moved to PicRef 
(PicRef currently has no PixelFormat field) and a couple of filters - 
overlay, vsrc_buffer both use the format field in addition to the 
defaults.c code.

4. Changing the name AVFilterPic to AVFilterBuffer wouldn't be a big 
deal because no filter uses these and only a few lines of framework code 
in avfilter.c and defualts.c would need changing to work with the new name.

Please let me know what can be done. It looks like a common buffer 
struct with changes 2 and 4 should be a small patch that won't break any 
existing video filters.

Regards,
Hemanth



More information about the ffmpeg-devel mailing list