[FFmpeg-devel] [PATCH] libavfilter: pass QP table through the filter chain

[Stefano Sabatini]

On date Wednesday 2012-09-05 18:01:54 +0200, Michael Niedermayer encoded:
> On Wed, Sep 05, 2012 at 04:53:52PM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2012-09-05 12:46:43 +0200, Michael Niedermayer encoded:
> > > Any volunteers to port the pp and spp filters from libmpcodec?
> > > 
> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > ---
> > >  libavfilter/avcodec.c  |    8 ++++++++
> > >  libavfilter/avfilter.h |    2 ++
> > >  libavfilter/buffer.c   |   23 ++++++++++++++++++++++-
> > >  libavfilter/version.h  |    4 ++--
> > >  libavfilter/video.c    |    1 +
> > >  5 files changed, 35 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavfilter/avcodec.c b/libavfilter/avcodec.c
> > > index f452303..51de6f5 100644
> > > --- a/libavfilter/avcodec.c
> > > +++ b/libavfilter/avcodec.c
> > > @@ -42,6 +42,14 @@ int avfilter_copy_frame_props(AVFilterBufferRef *dst, const AVFrame *src)
> > >          dst->video->top_field_first     = src->top_field_first;
> > >          dst->video->key_frame           = src->key_frame;
> > >          dst->video->pict_type           = src->pict_type;
> > > +        av_freep(&dst->video->qp_table);
> > > +        dst->video->qp_stride = 0;
> > > +        if (src->qscale_table) {
> > > +            int qsize = src->qstride ? src->qstride * ((src->height+15)/16) : (src->width+15)/16;
> > > +            dst->video->qp_stride       = src->qstride;
> > > +            dst->video->qp_table        = av_malloc(qsize);
> > > +            memcpy(dst->video->qp_table, src->qscale_table, qsize);
> > 
> > What if malloc fails?
> 
> fixed
> 
> 
> > 
> > Also maybe we should factorize the code.
> 
> yes i wanted to look into that in a subsequent patch
> 
> 
> > 
> > > +        }
> > >          break;
> > >      case AVMEDIA_TYPE_AUDIO:
> > >          dst->audio->sample_rate         = src->sample_rate;
> > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > index c0575ce..5316559 100644
> > > --- a/libavfilter/avfilter.h
> > > +++ b/libavfilter/avfilter.h
> > > @@ -131,6 +131,8 @@ typedef struct AVFilterBufferRefVideoProps {
> > >      int top_field_first;        ///< field order
> > >      enum AVPictureType pict_type; ///< picture type of the frame
> > >      int key_frame;              ///< 1 -> keyframe, 0-> not
> > > +    int qp_stride;                ///< qp_table stride
> > > +    int8_t *qp_table;             ///< array of Quantization Parameters
> > 
> > "stride" is kinda an MPlayerism.
> > 
> > Some alternatives:
> > qp_table | qp_linesize
> > qp       | qp_linesize
> > qp_data  | qp_linesize
> > qp_data  | qp_data_linesize
> > qp_table | qp_table_linesize
> 
> qp alone is wrong, qp_data is not adding much information beyond qp
> the linesize vs stride is bikeshed, we use stride for the matching
> field in AVFrame though.
> Iam happy if people rename the field to what they prefer ...

Then what about the last:
qp_table | qp_table_linesize

or qp_table_stride if you prefer.

In AVCodecContext it is qscale_table/qstride, but this nomenclature is
not very self-consistent (it's not apparent the relation between the
two fields). Sorry to be boring on this side, but I think names matter.
 
> [...]
> > > @@ -173,7 +184,17 @@ void avfilter_copy_buffer_ref_props(AVFilterBufferRef *dst, AVFilterBufferRef *s
> > >      dst->pos             = src->pos;
> > >  
> > >      switch (src->type) {
> > > -    case AVMEDIA_TYPE_VIDEO: *dst->video = *src->video; break;
> > > +    case AVMEDIA_TYPE_VIDEO: {
> > > +        if(dst->video->qp_table)
> > > +            av_freep(&dst->video->qp_table);
> > > +        *dst->video = *src->video;
> > > +        if(dst->video->qp_table) {
> >               ^^^
> >               src->video->qp_table?
> 
> they are the same

Yes, but it looks strange to set a struct if it is already defined in
the *destination*.

...

Nit: if_(...) {

(It's a nit so feel free to discard if you don't care.)
-- 
FFmpeg = Forgiving Frightening Mournful Pure Enlightened Ghost