[FFmpeg-devel] [PATCH] sbcdec: do not unnecessarily set frame properties

[Anton Khirnov]

Quoting Michael Niedermayer (2020-03-16 21:58:52)
> On Wed, Jan 29, 2020 at 06:00:16PM +0100, Anton Khirnov wrote:
> > Decoders are supposed to export stream properties in AVCodecContext, the
> > AVFrame properties are set from those in ff_get_buffer().
> > ---
> >  libavcodec/sbcdec.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/sbcdec.c b/libavcodec/sbcdec.c
> > index d8ea6855fe..2ebde46627 100644
> > --- a/libavcodec/sbcdec.c
> > +++ b/libavcodec/sbcdec.c
> > @@ -324,6 +324,8 @@ static int sbc_decode_init(AVCodecContext *avctx)
> >      SBCDecContext *sbc = avctx->priv_data;
> >      int i, ch;
> >  
> > +    avctx->sample_fmt = AV_SAMPLE_FMT_S16P;
> > +
> >      sbc->frame.crc_ctx = av_crc_get_table(AV_CRC_8_EBU);
> >  
> >      memset(sbc->dsp.V, 0, sizeof(sbc->dsp.V));
> > @@ -348,9 +350,8 @@ static int sbc_decode_frame(AVCodecContext *avctx,
> >      if (frame_length <= 0)
> >          return frame_length;
> >  
> > -    avctx->channels =
> > -    frame->channels = sbc->frame.channels;
> > -    frame->format = AV_SAMPLE_FMT_S16P;
> > +    avctx->channels = sbc->frame.channels;
> > +
> 
> probably ok but the design of exporting data which describes the current frame
> in the main context instead of the frames context. It gives a moment pause
> not feeling that this is the ideal design

Perhaps, but that's how things evolved historically and how things now
are. One decoder behaving differently from all the others is confusing,
especially when there is no reason for it.

FWIW I would be totally in favor of reducing the use of AVCodecContext
variables (it's too much of a "god structure") and switch to using frame
variables for this. But it's a lot of boring work and someone would have
to do it.

-- 
Anton Khirnov