| Message ID | 20200129170016.24338-1-anton@khirnov.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] sbcdec: do not unnecessarily set frame properties | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
ping
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 [...]
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.
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 --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;