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

[Michael Niedermayer]

On Thu, Sep 06, 2012 at 10:56:18PM +0200, Stefano Sabatini wrote:
> 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

changed


> 
> 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*.

fixed

> 
> ...
> 
> Nit: if_(...) {

changed

applied

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120907/9868f8ec/attachment.asc>