diff mbox

[FFmpeg-devel] lavf/riff: Do not use a rogue twocc for adpcm_swf

Message ID CAB0OVGpqDyTQQ4Mvk1jvFyX1TuFyJYdkpKg0CSdhxXch3bExAg@mail.gmail.com
State New
Headers show

Commit Message

Carl Eugen Hoyos Sept. 17, 2016, 1:06 p.m. UTC 
2016-09-15 20:48 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
> On Thu, Sep 15, 2016 at 03:47:49PM +0200, Carl Eugen Hoyos wrote:
>> 2016-09-15 15:42 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
>> >> -    { AV_CODEC_ID_ADPCM_SWF,       ('S' << 8) + 'F' },
>> >>      /* HACK/FIXME: Does Vorbis in WAV/AVI have an (in)official ID? */
>> >>      { AV_CODEC_ID_VORBIS,          ('V' << 8) + 'o' },
>> >>      { AV_CODEC_ID_NONE,      0 },
>> >
>> > does this affect adpcm_swf in nut ?
>>
>> Yes, indeed.
>>
>> Is it possible to fix adpcm_swf in wav?
>
> maybe if block_align is set (and is constant)

Attached fixes encoding, decoding still has the issue
that the decoder doesn't like more than one packet.
(Iiuc, it works if I limit the packet size in the wav demuxer.)

Carl Eugen

Comments

Michael Niedermayer Sept. 17, 2016, 4:26 p.m. UTC | #1 
On Sat, Sep 17, 2016 at 03:06:57PM +0200, Carl Eugen Hoyos wrote:
> 2016-09-15 20:48 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
> > On Thu, Sep 15, 2016 at 03:47:49PM +0200, Carl Eugen Hoyos wrote:
> >> 2016-09-15 15:42 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
> >> >> -    { AV_CODEC_ID_ADPCM_SWF,       ('S' << 8) + 'F' },
> >> >>      /* HACK/FIXME: Does Vorbis in WAV/AVI have an (in)official ID? */
> >> >>      { AV_CODEC_ID_VORBIS,          ('V' << 8) + 'o' },
> >> >>      { AV_CODEC_ID_NONE,      0 },
> >> >
> >> > does this affect adpcm_swf in nut ?
> >>
> >> Yes, indeed.
> >>
> >> Is it possible to fix adpcm_swf in wav?
> >
> > maybe if block_align is set (and is constant)
> 
> Attached fixes encoding, decoding still has the issue
> that the decoder doesn't like more than one packet.
> (Iiuc, it works if I limit the packet size in the wav demuxer.)
> 
> Carl Eugen

>  adpcmenc.c |    1 +
>  1 file changed, 1 insertion(+)
> dccfa2d54e899c17dcb57b8e37658955dfa6ea9c  0001-lavc-adpcmenc-Set-block_align-for-adpcm_swf.patch
> From 168bc2f828ffd7f85c0db736e6097e4198854c5d Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> Date: Sat, 17 Sep 2016 15:01:19 +0200
> Subject: [PATCH] lavc/adpcmenc: Set block_align for adpcm_swf.
> 
> ---
>  libavcodec/adpcmenc.c |    1 +
>  1 file changed, 1 insertion(+)

LGTM

can you add a fate test?

thx

[...]
Carl Eugen Hoyos Sept. 17, 2016, 10:48 p.m. UTC | #2 
2016-09-17 18:26 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:

>> >> Is it possible to fix adpcm_swf in wav?
>> >
>> > maybe if block_align is set (and is constant)
>>
>> Attached fixes encoding, decoding still has the issue
>> that the decoder doesn't like more than one packet.
>> (Iiuc, it works if I limit the packet size in the wav demuxer.)

>>  adpcmenc.c |    1 +
>>  1 file changed, 1 insertion(+)
>> dccfa2d54e899c17dcb57b8e37658955dfa6ea9c  0001-lavc-adpcmenc-Set-block_align-for-adpcm_swf.patch
>> From 168bc2f828ffd7f85c0db736e6097e4198854c5d Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>> Date: Sat, 17 Sep 2016 15:01:19 +0200
>> Subject: [PATCH] lavc/adpcmenc: Set block_align for adpcm_swf.
>>
>> ---
>>  libavcodec/adpcmenc.c |    1 +
>>  1 file changed, 1 insertion(+)
>
> LGTM

It unfortunately still needs a decoder fix that I may not be
able to provide, I tested by reducing MAX_SIZE in wavdec.c

Carl Eugen
Paul B Mahol Sept. 17, 2016, 11 p.m. UTC | #3 
On 9/17/16, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sat, Sep 17, 2016 at 03:06:57PM +0200, Carl Eugen Hoyos wrote:
>> 2016-09-15 20:48 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
>> > On Thu, Sep 15, 2016 at 03:47:49PM +0200, Carl Eugen Hoyos wrote:
>> >> 2016-09-15 15:42 GMT+02:00 Michael Niedermayer
>> >> <michael@niedermayer.cc>:
>> >> >> -    { AV_CODEC_ID_ADPCM_SWF,       ('S' << 8) + 'F' },
>> >> >>      /* HACK/FIXME: Does Vorbis in WAV/AVI have an (in)official ID?
>> >> >> */
>> >> >>      { AV_CODEC_ID_VORBIS,          ('V' << 8) + 'o' },
>> >> >>      { AV_CODEC_ID_NONE,      0 },
>> >> >
>> >> > does this affect adpcm_swf in nut ?
>> >>
>> >> Yes, indeed.
>> >>
>> >> Is it possible to fix adpcm_swf in wav?
>> >
>> > maybe if block_align is set (and is constant)
>>
>> Attached fixes encoding, decoding still has the issue
>> that the decoder doesn't like more than one packet.
>> (Iiuc, it works if I limit the packet size in the wav demuxer.)
>>
>> Carl Eugen
>
>>  adpcmenc.c |    1 +
>>  1 file changed, 1 insertion(+)
>> dccfa2d54e899c17dcb57b8e37658955dfa6ea9c
>> 0001-lavc-adpcmenc-Set-block_align-for-adpcm_swf.patch
>> From 168bc2f828ffd7f85c0db736e6097e4198854c5d Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>> Date: Sat, 17 Sep 2016 15:01:19 +0200
>> Subject: [PATCH] lavc/adpcmenc: Set block_align for adpcm_swf.
>>
>> ---
>>  libavcodec/adpcmenc.c |    1 +
>>  1 file changed, 1 insertion(+)
>
> LGTM
>
> can you add a fate test?

This patch is invalid. Block align should be set to what ADPCM_SWF
uses in adpcm_encode_frame as pkt_size.
And ADPCM_SWF should not be special, it should use block_align as pkt_size.
Michael Niedermayer Sept. 18, 2016, 12:33 a.m. UTC | #4 
On Sun, Sep 18, 2016 at 01:00:01AM +0200, Paul B Mahol wrote:
> On 9/17/16, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sat, Sep 17, 2016 at 03:06:57PM +0200, Carl Eugen Hoyos wrote:
> >> 2016-09-15 20:48 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
> >> > On Thu, Sep 15, 2016 at 03:47:49PM +0200, Carl Eugen Hoyos wrote:
> >> >> 2016-09-15 15:42 GMT+02:00 Michael Niedermayer
> >> >> <michael@niedermayer.cc>:
> >> >> >> -    { AV_CODEC_ID_ADPCM_SWF,       ('S' << 8) + 'F' },
> >> >> >>      /* HACK/FIXME: Does Vorbis in WAV/AVI have an (in)official ID?
> >> >> >> */
> >> >> >>      { AV_CODEC_ID_VORBIS,          ('V' << 8) + 'o' },
> >> >> >>      { AV_CODEC_ID_NONE,      0 },
> >> >> >
> >> >> > does this affect adpcm_swf in nut ?
> >> >>
> >> >> Yes, indeed.
> >> >>
> >> >> Is it possible to fix adpcm_swf in wav?
> >> >
> >> > maybe if block_align is set (and is constant)
> >>
> >> Attached fixes encoding, decoding still has the issue
> >> that the decoder doesn't like more than one packet.
> >> (Iiuc, it works if I limit the packet size in the wav demuxer.)
> >>
> >> Carl Eugen
> >
> >>  adpcmenc.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >> dccfa2d54e899c17dcb57b8e37658955dfa6ea9c
> >> 0001-lavc-adpcmenc-Set-block_align-for-adpcm_swf.patch
> >> From 168bc2f828ffd7f85c0db736e6097e4198854c5d Mon Sep 17 00:00:00 2001
> >> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> >> Date: Sat, 17 Sep 2016 15:01:19 +0200
> >> Subject: [PATCH] lavc/adpcmenc: Set block_align for adpcm_swf.
> >>
> >> ---
> >>  libavcodec/adpcmenc.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >
> > LGTM
> >
> > can you add a fate test?
> 
> This patch is invalid. Block align should be set to what ADPCM_SWF
> uses in adpcm_encode_frame as pkt_size.

yes


> And ADPCM_SWF should not be special, it should use block_align as pkt_size.

yes

[...]
diff mbox

Patch

From 168bc2f828ffd7f85c0db736e6097e4198854c5d Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <cehoyos@ag.or.at>
Date: Sat, 17 Sep 2016 15:01:19 +0200
Subject: [PATCH] lavc/adpcmenc: Set block_align for adpcm_swf.

---
 libavcodec/adpcmenc.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/adpcmenc.c b/libavcodec/adpcmenc.c
index 36974fd..8cf0d67 100644
--- a/libavcodec/adpcmenc.c
+++ b/libavcodec/adpcmenc.c
@@ -138,6 +138,7 @@  static av_cold int adpcm_encode_init(AVCodecContext *avctx)
             goto error;
         }
         avctx->frame_size = 512 * (avctx->sample_rate / 11025);
+        avctx->block_align = (256 * avctx->channels * avctx->sample_rate / 11025) + 1 + 2 * avctx->channels;
         break;
     default:
         ret = AVERROR(EINVAL);
-- 
1.7.10.4