diff mbox series

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

Message ID 20200129170016.24338-1-anton@khirnov.net
State Accepted
Headers show
Series [FFmpeg-devel] sbcdec: do not unnecessarily set frame properties
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Anton Khirnov Jan. 29, 2020, 5 p.m. UTC
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(-)

Comments

Anton Khirnov March 16, 2020, 8:23 a.m. UTC | #1
ping
Michael Niedermayer March 16, 2020, 8:58 p.m. UTC | #2
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

thx

[...]
Anton Khirnov March 18, 2020, 8:35 a.m. UTC | #3
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.
Michael Niedermayer March 18, 2020, 9:40 p.m. UTC | #4
On Wed, Mar 18, 2020 at 09:35:56AM +0100, Anton Khirnov wrote:
> 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.

i agree with everything you said above

thx

[...]
diff mbox series

Patch

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;
+
     frame->nb_samples = sbc->frame.blocks * sbc->frame.subbands;
     if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
         return ret;