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

[Stefano Sabatini]

On date Friday 2010-07-16 01:01:39 -0700, S.N. Hemanth Meenakshisundaram encoded:
> On 07/16/2010 01:00 AM, S.N. Hemanth Meenakshisundaram wrote:
>> On 07/16/2010 12:58 AM, S.N. Hemanth Meenakshisundaram wrote:
>>> On 07/15/2010 04:52 AM, S.N. Hemanth Meenakshisundaram wrote:
>>>> On 07/14/2010 07:51 AM, Michael Niedermayer wrote:
>>>>> [...]
>>>>> to elaborate on this, we need patches that apply to svn.
>>>>> you can send a patch series so that patch n depends on patches 0..n-1
>>>>> to be applied before it.
>>>>> but if patch x (x<n) is changed due to discussions all later patches
>>>>> must be rebased on the new code. We dont apply bad patches and then
>>>>> apply fixes on top.
>>>>>
>>>>>    
>>>
>>> [...]
>>>
>>> Am sending the series of patches again with the changes pointed out  
>>> earlier. [...]
>
> This is for formats enum to int and AV_PERM movement.

Applied the AV_PERM movement part.

Please regenerate the patch against latest SVN.

> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> index c8db36f..50b558e 100644
> --- a/libavfilter/avfilter.h
> +++ b/libavfilter/avfilter.h
> @@ -71,7 +71,7 @@ typedef struct AVFilterBuffer
>  {
>      uint8_t *data[4];           ///< picture data for each plane
>      int linesize[4];            ///< number of bytes per line
> -    enum PixelFormat format;    ///< colorspace
> +    int format;                 ///< colorspace

Update docs, that may say: colorspace or sample format

Also I'm not very happy about the use of an int. What about something
like this:

struct AVFilterBuffer
{
   enum AVMediaType type;
   ...

   union {
       enum PixelFormat  video_format;
       enum SampleFormat audio_format;
   } format;
   ...
}

?

  
>      unsigned refcount;          ///< number of references to this image
>  
> @@ -95,6 +95,11 @@ typedef struct AVFilterBuffer
>   *
>   * TODO: add anything necessary for frame reordering
>   */
> +#define AV_PERM_READ     0x01   ///< can read from the buffer
> +#define AV_PERM_WRITE    0x02   ///< can write to the buffer
> +#define AV_PERM_PRESERVE 0x04   ///< nobody else can overwrite the buffer
> +#define AV_PERM_REUSE    0x08   ///< can output the buffer multiple times, with the same contents each time
> +#define AV_PERM_REUSE2   0x10   ///< can output the buffer multiple times, modified each time
>  typedef struct AVFilterPicRef
>  {
>      AVFilterBuffer *pic;        ///< the picture that this is a reference to
> @@ -109,11 +114,6 @@ typedef struct AVFilterPicRef
>      AVRational pixel_aspect;    ///< pixel aspect ratio
>  
>      int perms;                  ///< permissions
> -#define AV_PERM_READ     0x01   ///< can read from the buffer
> -#define AV_PERM_WRITE    0x02   ///< can write to the buffer
> -#define AV_PERM_PRESERVE 0x04   ///< nobody else can overwrite the buffer
> -#define AV_PERM_REUSE    0x08   ///< can output the buffer multiple times, with the same contents each time
> -#define AV_PERM_REUSE2   0x10   ///< can output the buffer multiple times, modified each time
>  
>      int interlaced;             ///< is frame interlaced
>      int top_field_first;
> @@ -180,7 +180,7 @@ typedef struct AVFilterFormats AVFilterFormats;
>  struct AVFilterFormats
>  {
>      unsigned format_count;      ///< number of formats
> -    enum PixelFormat *formats;  ///< list of pixel formats
> +    int *formats;               ///< list of pixel formats

Again the same trick may be applied.
  
>      unsigned refcount;          ///< number of references to this list
>      AVFilterFormats ***refs;    ///< references to this list
> @@ -189,10 +189,10 @@ struct AVFilterFormats
>  /**
>   * Create a list of supported formats. This is intended for use in
>   * AVFilter->query_formats().
> - * @param pix_fmts list of pixel formats, terminated by PIX_FMT_NONE
> + * @param fmts list of pixel formats, terminated by PIX_FMT_NONE
>   * @return the format list, with no existing references
>   */
> -AVFilterFormats *avfilter_make_format_list(const enum PixelFormat *pix_fmts);
> +AVFilterFormats *avfilter_make_format_list(const int *fmts);
>  
>  /**
>   * Add pix_fmt to the list of pixel formats contained in *avff.
> @@ -499,7 +499,7 @@ struct AVFilterLink
>  
>      int w;                      ///< agreed upon image width
>      int h;                      ///< agreed upon image height
> -    enum PixelFormat format;    ///< agreed upon image colorspace
> +    int format;                 ///< agreed upon image colorspace
>  
>      /**
>       * Lists of formats supported by the input and output filters respectively.
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index 2a9bdb0..bd1086f 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -70,18 +70,18 @@ AVFilterFormats *avfilter_merge_formats(AVFilterFormats *a, AVFilterFormats *b)
>      return ret;
>  }
>  
> -AVFilterFormats *avfilter_make_format_list(const enum PixelFormat *pix_fmts)
> +AVFilterFormats *avfilter_make_format_list(const int *fmts)
>  {
>      AVFilterFormats *formats;
>      int count;

Maybe we could have a sort of generalization of the format concept.
Something along the line of:

typedef struct {
    enum AVMediaType type;    
    union {
        enum SampleFormat audio_format;
        enum PixelFormat  video_format;
    } format;
}

Opinions?

> -    for (count = 0; pix_fmts[count] != PIX_FMT_NONE; count++)
> +    for (count = 0; fmts[count] != PIX_FMT_NONE; count++)
>          ;

I don't like much this PIX_FMT_NONE, eventually it should be replaced
by the literal value -1 if we don't go for the AVFilterFormat thing.
  
>      formats               = av_mallocz(sizeof(AVFilterFormats));
>      formats->formats      = av_malloc(sizeof(*formats->formats) * count);
>      formats->format_count = count;
> -    memcpy(formats->formats, pix_fmts, sizeof(*formats->formats) * count);
> +    memcpy(formats->formats, fmts, sizeof(*formats->formats) * count);
>  
>      return formats;
>  }

Regards.