| Message ID | 20200410122641.14437-1-zane@zanevaniperen.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v3,1/2] avcodec: add adpcm_ima_ssi encoder | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On Fri, 10 Apr 2020 12:26:50 +0000 "Zane van Iperen" <zane@zanevaniperen.com> wrote: > Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> > --- > Changelog | 1 + > doc/general.texi | 2 +- > libavcodec/Makefile | 1 + > libavcodec/adpcmenc.c | 30 ++++++++++++++++++++++++++++++ > libavcodec/allcodecs.c | 1 + > libavcodec/utils.c | 1 + > libavcodec/version.h | 2 +- > 7 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/Changelog b/Changelog > index b0c016185e..f84420c6eb 100644 > > + if (avctx->trellis && avctx->codec->id == > AV_CODEC_ID_ADPCM_IMA_SSI) { > + /* > + * Trellis sort-of works, but has some DC offset problems. > + * Disable it until I can figure out why. > + */ > + av_log(avctx, AV_LOG_ERROR, "trellis not supported\n"); > + return AVERROR(EINVAL); > + } > + Ping. Also, could someone please clarify something for me? When encoding, there's no inherit differences between trellis and non-trellis other then a potentially more-accurate set of nibbles? And the decoder is none the wiser and chews on them both the same way? If I'm right, then this looks to be a decoder issue... Zane
On 4/11/20, Zane van Iperen <zane@zanevaniperen.com> wrote: > On Fri, 10 Apr 2020 12:26:50 +0000 > "Zane van Iperen" <zane@zanevaniperen.com> wrote: > >> Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> >> --- >> Changelog | 1 + >> doc/general.texi | 2 +- >> libavcodec/Makefile | 1 + >> libavcodec/adpcmenc.c | 30 ++++++++++++++++++++++++++++++ >> libavcodec/allcodecs.c | 1 + >> libavcodec/utils.c | 1 + >> libavcodec/version.h | 2 +- >> 7 files changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/Changelog b/Changelog >> index b0c016185e..f84420c6eb 100644 >> >> + if (avctx->trellis && avctx->codec->id == >> AV_CODEC_ID_ADPCM_IMA_SSI) { >> + /* >> + * Trellis sort-of works, but has some DC offset problems. >> + * Disable it until I can figure out why. >> + */ >> + av_log(avctx, AV_LOG_ERROR, "trellis not supported\n"); >> + return AVERROR(EINVAL); >> + } >> + > > Ping. > > Also, could someone please clarify something for me? > > When encoding, there's no inherit differences between trellis and > non-trellis other then a potentially more-accurate set of nibbles? > > And the decoder is none the wiser and chews on them both the same way? > > If I'm right, then this looks to be a decoder issue... > You are wrong, just follow qt adpcm encoder code. > > Zane > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, 11 Apr 2020 22:02:26 +0200 "Paul B Mahol" <onemda@gmail.com> wrote: > > > > Ping. > > > > Also, could someone please clarify something for me? > > > > When encoding, there's no inherit differences between trellis and > > non-trellis other then a potentially more-accurate set of nibbles? > > > > And the decoder is none the wiser and chews on them both the same > > way? > > > > If I'm right, then this looks to be a decoder issue... > > > > You are wrong, just follow qt adpcm encoder code. > I did. Is it possible, and I don't suggest this lightly, that the trellis code is bugged, and errors accumulate over time? I ask this because all of the encoders (except adpcm_yamaha, which is different anyway) have either the full sample and/or the step index periodically written to the bitstream, which is then used to reset the state machine, thus errors don't accumulate. adpcm_ima_ssi doesn't have that, so it relies on the state being accurate through the entire stream. This would certainly explain the behaviour I'm seeing in certain cases (see below). In both images, the top stream is encoded with '-trellis 0', and the bottom with '-trellis 1': - https://0x0.st/iSdS.png This one's obvious where the problem is. - https://0x0.st/iSdQ.png This is more subtle, but the entire stream is offset slightly. It's more noticeable at the start and end. For reference, here's the code I'm using. You'll see it's basically the same as the adpcm_ima_qt code: if (avctx->trellis > 0) { FF_ALLOC_OR_GOTO(avctx, buf, n * avctx->channels, error); for (ch = 0; ch < avctx->channels; ch++) { adpcm_compress_trellis(avctx, samples + ch, buf + n * ch, c->status + ch, n, avctx->channels); c->status[ch].prev_sample = c->status[ch].predictor; } for (i = 0; i < n; i++) { for (ch = 0; ch < avctx->channels; ch++) { put_bits(&pb, 4, buf[n * ch + i]); } } av_free(buf); } Or maybe I just don't fully understand how trellis works. Zane
On 4/12/20, Zane van Iperen <zane@zanevaniperen.com> wrote: > On Sat, 11 Apr 2020 22:02:26 +0200 > "Paul B Mahol" <onemda@gmail.com> wrote: > >> > >> > Ping. >> > >> > Also, could someone please clarify something for me? >> > >> > When encoding, there's no inherit differences between trellis and >> > non-trellis other then a potentially more-accurate set of nibbles? >> > >> > And the decoder is none the wiser and chews on them both the same >> > way? >> > >> > If I'm right, then this looks to be a decoder issue... >> > >> >> You are wrong, just follow qt adpcm encoder code. >> > > I did. > > > Is it possible, and I don't suggest this lightly, that the trellis code > is bugged, and errors accumulate over time? > > I ask this because all of the encoders (except adpcm_yamaha, which is > different anyway) have either the full sample and/or the step index > periodically written to the bitstream, which is then used to reset the > state machine, thus errors don't accumulate. > > adpcm_ima_ssi doesn't have that, so it relies on the state being > accurate through the entire stream. This would certainly explain the > behaviour I'm seeing in certain cases (see below). > > In both images, the top stream is encoded with '-trellis 0', and the > bottom with '-trellis 1': > - https://0x0.st/iSdS.png > This one's obvious where the problem is. > - https://0x0.st/iSdQ.png > This is more subtle, but the entire stream is offset slightly. It's > more noticeable at the start and end. > > > For reference, here's the code I'm using. You'll see it's basically > the same as the adpcm_ima_qt code: > > if (avctx->trellis > 0) { > FF_ALLOC_OR_GOTO(avctx, buf, n * avctx->channels, error); > > for (ch = 0; ch < avctx->channels; ch++) { > adpcm_compress_trellis(avctx, samples + ch, buf + n * ch, > c->status + ch, n, avctx->channels); > c->status[ch].prev_sample = c->status[ch].predictor; > } > > for (i = 0; i < n; i++) { > for (ch = 0; ch < avctx->channels; ch++) { > put_bits(&pb, 4, buf[n * ch + i]); > } > } > av_free(buf); > } > > > Or maybe I just don't fully understand how trellis works. Yes. that is 100% correct. Also you ignored fact that you do not update nodes for SSI variant in compress trellis.
On Sun, 12 Apr 2020 11:41:40 +0200 "Paul B Mahol" <onemda@gmail.com> wrote: > On 4/12/20, Zane van Iperen <zane@zanevaniperen.com> wrote: > > On Sat, 11 Apr 2020 22:02:26 +0200 > > "Paul B Mahol" <onemda@gmail.com> wrote: > > > >> > > >> > Ping. > >> > > >> > Also, could someone please clarify something for me? > >> > > >> > When encoding, there's no inherit differences between trellis and > >> > non-trellis other then a potentially more-accurate set of > >> > nibbles? > >> > > >> > And the decoder is none the wiser and chews on them both the same > >> > way? > >> > > >> > If I'm right, then this looks to be a decoder issue... > >> > > >> > >> You are wrong, just follow qt adpcm encoder code. > >> > > > > I did. > > > > > > Is it possible, and I don't suggest this lightly, that the trellis > > code is bugged, and errors accumulate over time? > > > > I ask this because all of the encoders (except adpcm_yamaha, which > > is different anyway) have either the full sample and/or the step > > index periodically written to the bitstream, which is then used to > > reset the state machine, thus errors don't accumulate. > > > > adpcm_ima_ssi doesn't have that, so it relies on the state being > > accurate through the entire stream. This would certainly explain the > > behaviour I'm seeing in certain cases (see below). > > > > In both images, the top stream is encoded with '-trellis 0', and the > > bottom with '-trellis 1': > > - https://0x0.st/iSdS.png > > This one's obvious where the problem is. > > - https://0x0.st/iSdQ.png > > This is more subtle, but the entire stream is offset slightly. > > It's more noticeable at the start and end. > > > > > > For reference, here's the code I'm using. You'll see it's basically > > the same as the adpcm_ima_qt code: > > > > if (avctx->trellis > 0) { > > FF_ALLOC_OR_GOTO(avctx, buf, n * avctx->channels, error); > > > > for (ch = 0; ch < avctx->channels; ch++) { > > adpcm_compress_trellis(avctx, samples + ch, buf + n * ch, > > c->status + ch, n, avctx->channels); > > c->status[ch].prev_sample = c->status[ch].predictor; > > } > > > > for (i = 0; i < n; i++) { > > for (ch = 0; ch < avctx->channels; ch++) { > > put_bits(&pb, 4, buf[n * ch + i]); > > } > > } > > av_free(buf); > > } > > > > > > Or maybe I just don't fully understand how trellis works. > > Yes. that is 100% correct. > > Also you ignored fact that you do not update nodes for SSI variant in > compress trellis. I probably should have mentioned that yes, I had already added the appropriate SSI code to adpcm_compress_trellis().
On 4/12/20, Zane van Iperen <zane@zanevaniperen.com> wrote: > On Sun, 12 Apr 2020 11:41:40 +0200 > "Paul B Mahol" <onemda@gmail.com> wrote: > >> On 4/12/20, Zane van Iperen <zane@zanevaniperen.com> wrote: >> > On Sat, 11 Apr 2020 22:02:26 +0200 >> > "Paul B Mahol" <onemda@gmail.com> wrote: >> > >> >> > >> >> > Ping. >> >> > >> >> > Also, could someone please clarify something for me? >> >> > >> >> > When encoding, there's no inherit differences between trellis and >> >> > non-trellis other then a potentially more-accurate set of >> >> > nibbles? >> >> > >> >> > And the decoder is none the wiser and chews on them both the same >> >> > way? >> >> > >> >> > If I'm right, then this looks to be a decoder issue... >> >> > >> >> >> >> You are wrong, just follow qt adpcm encoder code. >> >> >> > >> > I did. >> > >> > >> > Is it possible, and I don't suggest this lightly, that the trellis >> > code is bugged, and errors accumulate over time? >> > >> > I ask this because all of the encoders (except adpcm_yamaha, which >> > is different anyway) have either the full sample and/or the step >> > index periodically written to the bitstream, which is then used to >> > reset the state machine, thus errors don't accumulate. >> > >> > adpcm_ima_ssi doesn't have that, so it relies on the state being >> > accurate through the entire stream. This would certainly explain the >> > behaviour I'm seeing in certain cases (see below). >> > >> > In both images, the top stream is encoded with '-trellis 0', and the >> > bottom with '-trellis 1': >> > - https://0x0.st/iSdS.png >> > This one's obvious where the problem is. >> > - https://0x0.st/iSdQ.png >> > This is more subtle, but the entire stream is offset slightly. >> > It's more noticeable at the start and end. >> > >> > >> > For reference, here's the code I'm using. You'll see it's basically >> > the same as the adpcm_ima_qt code: >> > >> > if (avctx->trellis > 0) { >> > FF_ALLOC_OR_GOTO(avctx, buf, n * avctx->channels, error); >> > >> > for (ch = 0; ch < avctx->channels; ch++) { >> > adpcm_compress_trellis(avctx, samples + ch, buf + n * ch, >> > c->status + ch, n, avctx->channels); >> > c->status[ch].prev_sample = c->status[ch].predictor; >> > } >> > >> > for (i = 0; i < n; i++) { >> > for (ch = 0; ch < avctx->channels; ch++) { >> > put_bits(&pb, 4, buf[n * ch + i]); >> > } >> > } >> > av_free(buf); >> > } >> > >> > >> > Or maybe I just don't fully understand how trellis works. >> >> Yes. that is 100% correct. >> >> Also you ignored fact that you do not update nodes for SSI variant in >> compress trellis. > > I probably should have mentioned that yes, I had already added the > appropriate SSI code to adpcm_compress_trellis(). The one that updates node predictor? > > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, 12 Apr 2020 15:01:39 +0200 "Paul B Mahol" <onemda@gmail.com> wrote: > > > > I probably should have mentioned that yes, I had already added the > > appropriate SSI code to adpcm_compress_trellis(). > > The one that updates node predictor? > > > > Yep, wherever there was a check for QT, I added a check for SSI so that they both take the same code paths. They're the same format, so that shouldn't cause any problems.
On 4/12/20, Zane van Iperen <zane@zanevaniperen.com> wrote: > On Sun, 12 Apr 2020 15:01:39 +0200 > "Paul B Mahol" <onemda@gmail.com> wrote: > >> > >> > I probably should have mentioned that yes, I had already added the >> > appropriate SSI code to adpcm_compress_trellis(). >> >> The one that updates node predictor? >> > >> > > Yep, wherever there was a check for QT, I added a check for SSI so that > they both take the same code paths. > > They're the same format, so that shouldn't cause any problems. > Please point to latest such patch that contain that change. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, 12 Apr 2020 16:38:06 +0200 "Paul B Mahol" <onemda@gmail.com> wrote: > On 4/12/20, Zane van Iperen <zane@zanevaniperen.com> wrote: > > On Sun, 12 Apr 2020 15:01:39 +0200 > > "Paul B Mahol" <onemda@gmail.com> wrote: > > > >> > > >> > I probably should have mentioned that yes, I had already added > >> > the appropriate SSI code to adpcm_compress_trellis(). > >> > >> The one that updates node predictor? > >> > > >> > > > Yep, wherever there was a check for QT, I added a check for SSI so > > that they both take the same code paths. > > > > They're the same format, so that shouldn't cause any problems. > > > > Please point to latest such patch that contain that change. Here: https://github.com/vs49688/FFmpeg ssienc-v4-pre
diff --git a/Changelog b/Changelog index b0c016185e..f84420c6eb 100644 --- a/Changelog +++ b/Changelog @@ -57,6 +57,7 @@ version <next>: - overlay_cuda filter - switch from AvxSynth to AviSynth+ on Linux - mv30 decoder +- Simon & Schuster Interactive ADPCM encoder version 4.2: diff --git a/doc/general.texi b/doc/general.texi index 4adcc9e742..a7b0cc1718 100644 --- a/doc/general.texi +++ b/doc/general.texi @@ -1107,7 +1107,7 @@ following image formats are supported: @item ADPCM IMA Funcom @tab @tab X @item ADPCM IMA High Voltage Software ALP @tab @tab X @item ADPCM IMA QuickTime @tab X @tab X -@item ADPCM IMA Simon & Schuster Interactive @tab @tab X +@item ADPCM IMA Simon & Schuster Interactive @tab X @tab X @item ADPCM IMA Ubisoft APM @tab @tab X @item ADPCM IMA Loki SDL MJPEG @tab @tab X @item ADPCM IMA WAV @tab X @tab X diff --git a/libavcodec/Makefile b/libavcodec/Makefile index abb4992b60..52b566fcfe 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -852,6 +852,7 @@ OBJS-$(CONFIG_ADPCM_IMA_QT_DECODER) += adpcm.o adpcm_data.o OBJS-$(CONFIG_ADPCM_IMA_QT_ENCODER) += adpcmenc.o adpcm_data.o OBJS-$(CONFIG_ADPCM_IMA_RAD_DECODER) += adpcm.o adpcm_data.o OBJS-$(CONFIG_ADPCM_IMA_SSI_DECODER) += adpcm.o adpcm_data.o +OBJS-$(CONFIG_ADPCM_IMA_SSI_ENCODER) += adpcmenc.o adpcm_data.o OBJS-$(CONFIG_ADPCM_IMA_SMJPEG_DECODER) += adpcm.o adpcm_data.o OBJS-$(CONFIG_ADPCM_IMA_WAV_DECODER) += adpcm.o adpcm_data.o OBJS-$(CONFIG_ADPCM_IMA_WAV_ENCODER) += adpcmenc.o adpcm_data.o diff --git a/libavcodec/adpcmenc.c b/libavcodec/adpcmenc.c index 668939c778..1d6ea798a9 100644 --- a/libavcodec/adpcmenc.c +++ b/libavcodec/adpcmenc.c @@ -77,6 +77,15 @@ static av_cold int adpcm_encode_init(AVCodecContext *avctx) return AVERROR(EINVAL); } + if (avctx->trellis && avctx->codec->id == AV_CODEC_ID_ADPCM_IMA_SSI) { + /* + * Trellis sort-of works, but has some DC offset problems. + * Disable it until I can figure out why. + */ + av_log(avctx, AV_LOG_ERROR, "trellis not supported\n"); + return AVERROR(EINVAL); + } + if (avctx->trellis) { int frontier = 1 << avctx->trellis; int max_paths = frontier * FREEZE_INTERVAL; @@ -139,6 +148,10 @@ static av_cold int adpcm_encode_init(AVCodecContext *avctx) } avctx->frame_size = 512 * (avctx->sample_rate / 11025); break; + case AV_CODEC_ID_ADPCM_IMA_SSI: + avctx->frame_size = BLKSIZE; + avctx->block_align = BLKSIZE; + break; default: ret = AVERROR(EINVAL); goto error; @@ -568,6 +581,22 @@ static int adpcm_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, flush_put_bits(&pb); break; } + case AV_CODEC_ID_ADPCM_IMA_SSI: + { + PutBitContext pb; + init_put_bits(&pb, dst, pkt_size); + + av_assert0(avctx->trellis == 0); + + for (i = 0; i < frame->nb_samples; i++) { + for (ch = 0; ch < avctx->channels; ch++) { + put_bits(&pb, 4, adpcm_ima_qt_compress_sample(c->status + ch, *samples++)); + } + } + + flush_put_bits(&pb); + break; + } case AV_CODEC_ID_ADPCM_SWF: { PutBitContext pb; @@ -720,6 +749,7 @@ AVCodec ff_ ## name_ ## _encoder = { \ } ADPCM_ENCODER(AV_CODEC_ID_ADPCM_IMA_QT, adpcm_ima_qt, sample_fmts_p, "ADPCM IMA QuickTime"); +ADPCM_ENCODER(AV_CODEC_ID_ADPCM_IMA_SSI, adpcm_ima_ssi, sample_fmts, "ADPCM IMA Simon & Schuster Interactive"); ADPCM_ENCODER(AV_CODEC_ID_ADPCM_IMA_WAV, adpcm_ima_wav, sample_fmts_p, "ADPCM IMA WAV"); ADPCM_ENCODER(AV_CODEC_ID_ADPCM_MS, adpcm_ms, sample_fmts, "ADPCM Microsoft"); ADPCM_ENCODER(AV_CODEC_ID_ADPCM_SWF, adpcm_swf, sample_fmts, "ADPCM Shockwave Flash"); diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c index e7b29426db..b4e0d65052 100644 --- a/libavcodec/allcodecs.c +++ b/libavcodec/allcodecs.c @@ -617,6 +617,7 @@ extern AVCodec ff_adpcm_ima_qt_encoder; extern AVCodec ff_adpcm_ima_qt_decoder; extern AVCodec ff_adpcm_ima_rad_decoder; extern AVCodec ff_adpcm_ima_ssi_decoder; +extern AVCodec ff_adpcm_ima_ssi_encoder; extern AVCodec ff_adpcm_ima_smjpeg_decoder; extern AVCodec ff_adpcm_ima_wav_encoder; extern AVCodec ff_adpcm_ima_wav_decoder; diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 4ab8cff4f5..b297eb3552 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1463,6 +1463,7 @@ int av_get_exact_bits_per_sample(enum AVCodecID codec_id) case AV_CODEC_ID_ADPCM_IMA_EA_SEAD: case AV_CODEC_ID_ADPCM_IMA_OKI: case AV_CODEC_ID_ADPCM_IMA_WS: + case AV_CODEC_ID_ADPCM_IMA_SSI: case AV_CODEC_ID_ADPCM_G722: case AV_CODEC_ID_ADPCM_YAMAHA: case AV_CODEC_ID_ADPCM_AICA: diff --git a/libavcodec/version.h b/libavcodec/version.h index e62d1a7925..a4eb83124c 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -28,7 +28,7 @@ #include "libavutil/version.h" #define LIBAVCODEC_VERSION_MAJOR 58 -#define LIBAVCODEC_VERSION_MINOR 78 +#define LIBAVCODEC_VERSION_MINOR 79 #define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> --- Changelog | 1 + doc/general.texi | 2 +- libavcodec/Makefile | 1 + libavcodec/adpcmenc.c | 30 ++++++++++++++++++++++++++++++ libavcodec/allcodecs.c | 1 + libavcodec/utils.c | 1 + libavcodec/version.h | 2 +- 7 files changed, 36 insertions(+), 2 deletions(-)