[FFmpeg-devel] [PATCH] avcodec/wmaprodec: improve WMAPRO/XMA gapless output

Rostislav Pehlivanov atomnuker at gmail.com
Wed Oct 10 20:57:11 EEST 2018


On Wed, 10 Oct 2018 at 00:15, <bananaman255 at gmail.com> wrote:

> From: bnnm <bananaman255 at gmail.com>
>
> Improves trac issue #6722. Fixes truncated WMAPRO/XMA output due to delay
> samples and partially applies bitstream gapless info.
>
> This can be tested with files encoding a small nb_samples (like 400),
> which couldn't output anything due to missing delay at the end.
>
> Applying end_skip and partial delay samples would require some extra
> changes, so files are slightly bigger than what should be.
>

> Signed-off-by: bnnm <bananaman255 at gmail.com>
> ---
>  libavcodec/wmaprodec.c | 100
> ++++++++++++++++++++++++++++++++++++-------------
>  tests/fate/wma.mak     |   1 +
>  2 files changed, 74 insertions(+), 27 deletions(-)
>
> diff --git a/libavcodec/wmaprodec.c b/libavcodec/wmaprodec.c
> index 9439bfa771..e18e2e438f 100644
> --- a/libavcodec/wmaprodec.c
> +++ b/libavcodec/wmaprodec.c
> @@ -216,9 +216,9 @@ typedef struct WMAProDecodeCtx {
>      GetBitContext    gb;                            ///< bitstream reader
> context
>      int              buf_bit_size;                  ///< buffer size in
> bits
>      uint8_t          drc_gain;                      ///< gain for the DRC
> tool
> -    int8_t           skip_frame;                    ///< skip output step
>      int8_t           parsed_all_subframes;          ///< all subframes
> decoded?
>      uint8_t          skip_packets;                  ///< packets to skip
> to find next packet in a stream (XMA1/2)
> +    int8_t           eof_done;                      ///< set when EOF
> reached and extra subframe is written (XMA1/2)
>

>      /* subframe/block decode state */
>      int16_t          subframe_len;                  ///< current subframe
> length
> @@ -379,12 +379,6 @@ static av_cold int decode_init(WMAProDecodeCtx *s,
> AVCodecContext *avctx, int nu
>          return AVERROR_PATCHWELCOME;
>      }
>
> -    /** frame info */
> -    if (avctx->codec_id != AV_CODEC_ID_WMAPRO)
> -        s->skip_frame = 0;
> -    else
> -        s->skip_frame = 1; /* skip first frame */
> -
>      s->packet_loss = 1;
>      s->len_prefix  = (s->decode_flags & 0x40);
>
> @@ -1450,21 +1444,38 @@ static int decode_frame(WMAProDecodeCtx *s,
> AVFrame *frame, int *got_frame_ptr)
>          ff_dlog(s->avctx, "drc_gain %i\n", s->drc_gain);
>      }
>
> -    /** no idea what these are for, might be the number of samples
> -        that need to be skipped at the beginning or end of a stream */
> +    /** read encoder delay/padding (gapless) info */
>      if (get_bits1(gb)) {
> -        int av_unused skip;
> +        int start_skip, end_skip;
> +
>
> -        /** usually true for the first frame */
> +        /** usually true for the first frame and equal to 1 frame */
>

Usually true? See below.


         if (get_bits1(gb)) {
> -            skip = get_bits(gb, av_log2(s->samples_per_frame * 2));
> -            ff_dlog(s->avctx, "start skip: %i\n", skip);
> +            start_skip = get_bits(gb, av_log2(s->samples_per_frame * 2));
> +            start_skip = FFMIN(start_skip, s->samples_per_frame);
> +
> +            /* must add for XMA to respect starting skip_samples */
> +            s->avctx->internal->skip_samples += start_skip;
>

No, you need to set this during init.
You should also set avctx->delay to the same value.



>          }
>
> -        /** sometimes true for the last frame */
> +        /** usually true for the last frame and less than 1 frame */


Usually true? Decoders aren't usually true, they're always true.


         if (get_bits1(gb)) {
>
-            skip = get_bits(gb, av_log2(s->samples_per_frame * 2));
> -            ff_dlog(s->avctx, "end skip: %i\n", skip);
> +            end_skip = get_bits(gb, av_log2(s->samples_per_frame * 2));
> +            end_skip = FFMIN(end_skip, s->samples_per_frame);
> +
> +            //TODO fix end_skip (also needs changes in XMA multistream's
> handling)
> +            /* for XMA/WMAPRO end_skip may span last frame + final delay
> samples,
> +             * so we'd need some counter to (possibly) skip some in this
> frame and
> +             * rest after last frame (buf_size 0), so final samples would
> be:
> +             * samples_per_frame + delay_samples - start_skip - end_skip
> +             * (files with 1 packet are possible so both skips are
> considered).
> +             *
> +             * WMAPRO 5.1 in XWMA does't do this and audio is padded
> instead,
> +             * and min packets is 2 (assumes only 1/2ch need it). */
> +
> +            if (s->avctx->codec_id == AV_CODEC_ID_WMAPRO &&
> s->nb_channels > 2) {
> +                frame->nb_samples = s->samples_per_frame - end_skip;
> +            }
>

No, this is wrong. Codecs don't need to do this manually, this is done by
libavcodec/decode.c, based on the packet's AV_PKT_DATA_SKIP_SAMPLES side
data.


         }
>
>      }
> @@ -1500,13 +1511,9 @@ static int decode_frame(WMAProDecodeCtx *s, AVFrame
> *frame, int *got_frame_ptr)
>                 s->samples_per_frame * sizeof(*s->channel[i].out) >> 1);
>      }
>
> -    if (s->skip_frame) {
> -        s->skip_frame = 0;
> -        *got_frame_ptr = 0;
> -        av_frame_unref(frame);
> -    } else {
> -        *got_frame_ptr = 1;
> -    }
> +    /** frame may be partially discarded with gapless info in the
> bitstream */
> +    *got_frame_ptr = 1;
> +
>
>      if (s->len_prefix) {
>          if (len != (get_bits_count(gb) - s->frame_offset) + 2) {
> @@ -1609,7 +1616,35 @@ static int decode_packet(AVCodecContext *avctx,
> WMAProDecodeCtx *s,
>
>      *got_frame_ptr = 0;
>
> -    if (s->packet_done || s->packet_loss) {
> +    if (!buf_size) {
> +        AVFrame *frame = data;
> +        int i;
> +
> +        /** Must output remaining samples after stream end. WMAPRO 5.1
> created
> +         * by XWMA encoder don't though; assuming only 1/2ch streams need
> it. */
> +        s->packet_done = 0;
> +        if (s->eof_done || s->nb_channels > 2)
> +            return 0;
> +
> +        /** clean output buffer and copy last IMCDT samples */
> +        for (i = 0; i < s->nb_channels; i++) {
> +            memset(frame->extended_data[i], 0,
> +            s->samples_per_frame * sizeof(*s->channel[i].out));
> +
> +            memcpy(frame->extended_data[i], s->channel[i].out,
> +                   s->samples_per_frame * sizeof(*s->channel[i].out) >>
> 1);
> +        }
> +
> +        /* TODO: change nb_samples to delay_samples. XMA always uses 128
> +         * (with 512 samples per frame) and WMAPRO may use 768 (2048
> s.p.f.).
> +         * XMA needs changes in multi-stream handling though. */
> +
> +        s->eof_done = 1;
> +        s->packet_done = 1;
> +        *got_frame_ptr = 1;
> +        return 0;
> +    }
> +    else if (s->packet_done || s->packet_loss) {
>          s->packet_done = 0;
>
>          /** sanity check for the buffer length */
> @@ -1896,6 +1931,12 @@ static av_cold int xma_decode_init(AVCodecContext
> *avctx)
>          start_channels += s->xma[i].nb_channels;
>      }
>
> +    /** XMA seems to require this to create gapless files (WMAPRO encoder
> +     * pads files with silence, while XMA can encode any nb_samples).
> +     * This value seems to counter the extra subframe output at EOF
> +     * (or perhaps, XMA decoder should output 64 samples earlier?) */
> +    avctx->internal->skip_samples = 128/2;
>

This is confusing, you should split the patches affecting different codecs.



> +
>      return ret;
>  }
>
> @@ -1922,6 +1963,7 @@ static void flush(WMAProDecodeCtx *s)
>                 sizeof(*s->channel[i].out));
>      s->packet_loss = 1;
>      s->skip_packets = 0;
> +    s->eof_done = 0;
>  }
>
>
> @@ -1934,6 +1976,8 @@ static void wmapro_flush(AVCodecContext *avctx)
>      WMAProDecodeCtx *s = avctx->priv_data;
>
>      flush(s);
> +
> +    avctx->internal->skip_samples = 0;
>  }
>
>  static void xma_flush(AVCodecContext *avctx)
> @@ -1946,6 +1990,8 @@ static void xma_flush(AVCodecContext *avctx)
>
>      memset(s->offset, 0, sizeof(s->offset));
>      s->current_stream = 0;
> +
> +    avctx->internal->skip_samples = 128/2;
>  }
>
>
> @@ -1961,7 +2007,7 @@ AVCodec ff_wmapro_decoder = {
>      .init           = wmapro_decode_init,
>      .close          = wmapro_decode_end,
>      .decode         = wmapro_decode_packet,
> -    .capabilities   = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
> +    .capabilities   = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1 |
> AV_CODEC_CAP_DELAY,
>      .flush          = wmapro_flush,
>      .sample_fmts    = (const enum AVSampleFormat[]) { AV_SAMPLE_FMT_FLTP,
>                                                        AV_SAMPLE_FMT_NONE
> },
> @@ -1976,7 +2022,7 @@ AVCodec ff_xma1_decoder = {
>      .init           = xma_decode_init,
>      .close          = xma_decode_end,
>      .decode         = xma_decode_packet,
> -    .capabilities   = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
> +    .capabilities   = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1 |
> AV_CODEC_CAP_DELAY,
>      .sample_fmts    = (const enum AVSampleFormat[]) { AV_SAMPLE_FMT_FLTP,
>                                                        AV_SAMPLE_FMT_NONE
> },
>  };
> @@ -1991,7 +2037,7 @@ AVCodec ff_xma2_decoder = {
>      .close          = xma_decode_end,
>      .decode         = xma_decode_packet,
>      .flush          = xma_flush,
> -    .capabilities   = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
> +    .capabilities   = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1 |
> AV_CODEC_CAP_DELAY,
>      .sample_fmts    = (const enum AVSampleFormat[]) { AV_SAMPLE_FMT_FLTP,
>                                                        AV_SAMPLE_FMT_NONE
> },
>  };
> diff --git a/tests/fate/wma.mak b/tests/fate/wma.mak
> index 12a8fa989a..9c7ce6fd7d 100644
> --- a/tests/fate/wma.mak
> +++ b/tests/fate/wma.mak
> @@ -11,6 +11,7 @@ fate-wmapro-5.1: SIZE_TOLERANCE = 24576
>  FATE_WMAPRO-$(call DEMDEC, MOV, WMAPRO) += fate-wmapro-ism
>  fate-wmapro-ism: CMD = pcm -i $(TARGET_SAMPLES)/isom/vc1-wmapro.ism -vn
>  fate-wmapro-ism: REF = $(SAMPLES)/isom/vc1-wmapro.pcm
> +fate-wmapro-ism: SIZE_TOLERANCE = 12976
>
>  $(FATE_WMAPRO-yes): CMP = oneoff
>
> --
> 2.11.0.windows.3
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list