[PATCH] g722 decoder, no licensing fame
On Wed, Mar 25, 2009 at 2:39 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Mar 25, 2009 at 09:57:51AM -0700, Kenan Gillet wrote:
Hi,
On Mar 21, 2009, at 1:34 PM, Michael Niedermayer wrote:
On Sat, Mar 21, 2009 at 12:43:35AM -0700, Kenan Gillet wrote: [...]
@@ -2819,6 +2822,7 @@ memset(ap, 0, sizeof(*ap)); ap->prealloced_context = 1; ap->sample_rate = audio_sample_rate; + ap->bit_rate = audio_bit_rate; ap->channels = audio_channels; ap->time_base.den = frame_rate.num; ap->time_base.num = frame_rate.den; @@ -2892,6 +2896,7 @@ channel_layout = enc->channel_layout; audio_channels = enc->channels; audio_sample_rate = enc->sample_rate; + audio_bit_rate = enc->bit_rate; audio_sample_fmt = enc->sample_fmt; input_codecs[nb_icodecs++] = avcodec_find_decoder_by_name(audio_codec_name); if(audio_disable) @@ -3211,6 +3216,7 @@ } nb_ocodecs++; audio_enc->sample_rate = audio_sample_rate; + audio_enc->bit_rate = audio_bit_rate; audio_enc->time_base= (AVRational){1, audio_sample_rate}; if (audio_language) { av_metadata_set(&st->metadata, "language", audio_language);
Index: libavformat/avformat.h =================================================================== --- libavformat/avformat.h (revision 18096) +++ libavformat/avformat.h (working copy) @@ -261,6 +261,7 @@ enum CodecID video_codec_id; enum CodecID audio_codec_id; #endif + int bit_rate; } AVFormatParameters;
//! Demuxer will use url_fopen, no opened file should be provided by the caller.
the whole struct is pretty much deprecated and iam not happy if people add new things to it.
any hint on to "do it the right way" ?
AVFormatContext.bit_rate maybe but i am not sure if this might cause some issues
actually a much simpler solution would be to change the -ab options to set in the AVCodecContext.bitrate for decoder too like in the patch attached but it would change the behavior of ffmpeg but that's exactly what is needed, to be able to give the g722 decoder a bit_rate would that be acceptable? Kenan
Hi, On Thu, Mar 26, 2009 at 12:37 AM, Kenan Gillet <kenan.gillet at gmail.com> wrote:
actually a much simpler solution would be to change the -ab options to set in the AVCodecContext.bitrate for decoder too like in the patch attached [..] but that's exactly what is needed, to be able to give the g722 decoder a bit_rate
For many codecs, the demuxer sets it. Can't you deduce the bitrate in the demuxer somehow (i.e. from container) to automagically set AVCodecContext->bit_rate? Ronald
On Wed, Mar 25, 2009 at 09:37:58PM -0700, Kenan Gillet wrote:
On Wed, Mar 25, 2009 at 2:39 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Mar 25, 2009 at 09:57:51AM -0700, Kenan Gillet wrote:
Hi,
On Mar 21, 2009, at 1:34 PM, Michael Niedermayer wrote:
On Sat, Mar 21, 2009 at 12:43:35AM -0700, Kenan Gillet wrote: [...]
@@ -2819,6 +2822,7 @@ memset(ap, 0, sizeof(*ap)); ap->prealloced_context = 1; ap->sample_rate = audio_sample_rate; + ap->bit_rate = audio_bit_rate; ap->channels = audio_channels; ap->time_base.den = frame_rate.num; ap->time_base.num = frame_rate.den; @@ -2892,6 +2896,7 @@ channel_layout = enc->channel_layout; audio_channels = enc->channels; audio_sample_rate = enc->sample_rate; + audio_bit_rate = enc->bit_rate; audio_sample_fmt = enc->sample_fmt; input_codecs[nb_icodecs++] = avcodec_find_decoder_by_name(audio_codec_name); if(audio_disable) @@ -3211,6 +3216,7 @@ } nb_ocodecs++; audio_enc->sample_rate = audio_sample_rate; + audio_enc->bit_rate = audio_bit_rate; audio_enc->time_base= (AVRational){1, audio_sample_rate}; if (audio_language) { av_metadata_set(&st->metadata, "language", audio_language);
Index: libavformat/avformat.h =================================================================== --- libavformat/avformat.h (revision 18096) +++ libavformat/avformat.h (working copy) @@ -261,6 +261,7 @@ enum CodecID video_codec_id; enum CodecID audio_codec_id; #endif + int bit_rate; } AVFormatParameters;
//! Demuxer will use url_fopen, no opened file should be provided by the caller.
the whole struct is pretty much deprecated and iam not happy if people add new things to it.
any hint on to "do it the right way" ?
AVFormatContext.bit_rate maybe but i am not sure if this might cause some issues
actually a much simpler solution would be to change the -ab options to set in the AVCodecContext.bitrate for decoder too like in the patch attached
but it would change the behavior of ffmpeg
but that's exactly what is needed, to be able to give the g722 decoder a bit_rate
would that be acceptable?
have you checked that this doesnt break transcoding with multiple inputs and outputs, i mean the -ab must not mistakely override the wrong one [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No great genius has ever existed without some touch of madness. -- Aristotle
Hi On Thu, Mar 26, 2009 at 11:24 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Mar 25, 2009 at 09:37:58PM -0700, Kenan Gillet wrote:
On Wed, Mar 25, 2009 at 2:39 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Mar 25, 2009 at 09:57:51AM -0700, Kenan Gillet wrote:
Hi,
On Mar 21, 2009, at 1:34 PM, Michael Niedermayer wrote:
On Sat, Mar 21, 2009 at 12:43:35AM -0700, Kenan Gillet wrote: [...]
@@ -2819,6 +2822,7 @@ ? ? memset(ap, 0, sizeof(*ap)); ? ? ap->prealloced_context = 1; ? ? ap->sample_rate = audio_sample_rate; + ? ?ap->bit_rate = audio_bit_rate; ? ? ap->channels = audio_channels; ? ? ap->time_base.den = frame_rate.num; ? ? ap->time_base.num = frame_rate.den; @@ -2892,6 +2896,7 @@ ? ? ? ? ? ? channel_layout = enc->channel_layout; ? ? ? ? ? ? audio_channels = enc->channels; ? ? ? ? ? ? audio_sample_rate = enc->sample_rate; + ? ? ? ? ? ?audio_bit_rate = enc->bit_rate; ? ? ? ? ? ? audio_sample_fmt = enc->sample_fmt; ? ? ? ? ? ? input_codecs[nb_icodecs++] = avcodec_find_decoder_by_name(audio_codec_name); ? ? ? ? ? ? if(audio_disable) @@ -3211,6 +3216,7 @@ ? ? } ? ? nb_ocodecs++; ? ? audio_enc->sample_rate = audio_sample_rate; + ? ?audio_enc->bit_rate = audio_bit_rate; ? ? audio_enc->time_base= (AVRational){1, audio_sample_rate}; ? ? if (audio_language) { ? ? ? ? av_metadata_set(&st->metadata, "language", audio_language);
Index: libavformat/avformat.h =================================================================== --- libavformat/avformat.h ?(revision 18096) +++ libavformat/avformat.h ?(working copy) @@ -261,6 +261,7 @@ ? ? enum CodecID video_codec_id; ? ? enum CodecID audio_codec_id; #endif + ? ?int bit_rate; } AVFormatParameters;
//! Demuxer will use url_fopen, no opened file should be provided by the caller.
the whole struct is pretty much deprecated and iam not happy if people add new things to it.
any hint on to "do it the right way" ?
AVFormatContext.bit_rate maybe but i am not sure if this might cause some issues
actually a much simpler solution would be to change the -ab options to set in the AVCodecContext.bitrate for decoder too like in the patch attached
but it would change the behavior of ffmpeg
but that's exactly what is needed, to be able to give the g722 decoder a bit_rate
would that be acceptable?
have you checked that this doesnt break transcoding with multiple inputs and outputs, i mean the -ab must not mistakely override the wrong one
I tried with different samples and so far it works, but I am still working on making sure no regression happens by inspecting the audio decoder. In the meantime, the init function overwrites the AVCodecContext.bit_rate to the default g722 value of 64000 and issue a warning if the standard compliance is not set to strict. new patches attached Kenan
On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
new patches attached
--- libavcodec/Makefile (revision 18292) +++ libavcodec/Makefile (working copy) @@ -324,6 +324,8 @@ OBJS-$(CONFIG_ADPCM_EA_XAS_DECODER) += adpcm.o +OBJS-$(CONFIG_ADPCM_G722_DECODER) += g722.o +OBJS-$(CONFIG_ADPCM_G722_ENCODER) += g722.o
Maybe it would be worth splitting this into g722dec.c and g722enc.c. It would save some #ifdeffery. Diego
Hi, On Apr 1, 2009, at 8:58 AM, Diego Biurrun wrote:
On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
new patches attached
--- libavcodec/Makefile (revision 18292) +++ libavcodec/Makefile (working copy) @@ -324,6 +324,8 @@ OBJS-$(CONFIG_ADPCM_EA_XAS_DECODER) += adpcm.o +OBJS-$(CONFIG_ADPCM_G722_DECODER) += g722.o +OBJS-$(CONFIG_ADPCM_G722_ENCODER) += g722.o
Maybe it would be worth splitting this into g722dec.c and g722enc.c. It would save some #ifdeffery.
Decoder and encoder sharethe structure definition, most of the code and tables. g722.c would need to be split in g722.h, g722dec.c, g722enc.c and g722common.c. Are the 3 #if worth the extra complexity ? I kind of prefer a self-contained file, but have no strong feeling about it. thanks, Kenan
On Wed, Apr 01, 2009 at 10:15:18AM -0700, Kenan Gillet wrote:
On Apr 1, 2009, at 8:58 AM, Diego Biurrun wrote:
On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
new patches attached
--- libavcodec/Makefile (revision 18292) +++ libavcodec/Makefile (working copy) @@ -324,6 +324,8 @@ OBJS-$(CONFIG_ADPCM_EA_XAS_DECODER) += adpcm.o +OBJS-$(CONFIG_ADPCM_G722_DECODER) += g722.o +OBJS-$(CONFIG_ADPCM_G722_ENCODER) += g722.o
Maybe it would be worth splitting this into g722dec.c and g722enc.c. It would save some #ifdeffery.
Decoder and encoder sharethe structure definition, most of the code and tables. g722.c would need to be split in g722.h, g722dec.c, g722enc.c and g722common.c. Are the 3 #if worth the extra complexity ?
Possibly not. You could, however, move the decoder tables to the bottom of the file under the decoder #ifdef, that would save you one #ifdef. Feel free to ignore me if you prefer to sort it out another way. Diego
Hi, On Wed, Apr 1, 2009 at 3:10 PM, Diego Biurrun <diego at biurrun.de> wrote:
On Wed, Apr 01, 2009 at 10:15:18AM -0700, Kenan Gillet wrote:
On Apr 1, 2009, at 8:58 AM, Diego Biurrun wrote:
On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
new patches attached
--- libavcodec/Makefile ? ? (revision 18292) +++ libavcodec/Makefile ? ? (working copy) @@ -324,6 +324,8 @@ OBJS-$(CONFIG_ADPCM_EA_XAS_DECODER) ? ? ? += adpcm.o +OBJS-$(CONFIG_ADPCM_G722_DECODER) ? ? ? ? += g722.o +OBJS-$(CONFIG_ADPCM_G722_ENCODER) ? ? ? ? += g722.o
Maybe it would be worth splitting this into g722dec.c and g722enc.c. ?It would save some #ifdeffery.
Decoder and encoder sharethe structure definition, most of the code and tables. g722.c would need to be split in g722.h, g722dec.c, g722enc.c and g722common.c. Are the 3 #if worth the extra complexity ?
Possibly not. ?You could, however, move the decoder tables to the bottom of the file under the decoder #ifdef, that would save you one #ifdef. Feel free to ignore me if you prefer to sort it out another way.
thanks, changed locally Kenan
On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
Hi
On Thu, Mar 26, 2009 at 11:24 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Mar 25, 2009 at 09:37:58PM -0700, Kenan Gillet wrote:
On Wed, Mar 25, 2009 at 2:39 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Mar 25, 2009 at 09:57:51AM -0700, Kenan Gillet wrote:
Hi,
On Mar 21, 2009, at 1:34 PM, Michael Niedermayer wrote:
On Sat, Mar 21, 2009 at 12:43:35AM -0700, Kenan Gillet wrote: [...]
> @@ -2819,6 +2822,7 @@ > ? ? memset(ap, 0, sizeof(*ap)); > ? ? ap->prealloced_context = 1; > ? ? ap->sample_rate = audio_sample_rate; > + ? ?ap->bit_rate = audio_bit_rate; > ? ? ap->channels = audio_channels; > ? ? ap->time_base.den = frame_rate.num; > ? ? ap->time_base.num = frame_rate.den; > @@ -2892,6 +2896,7 @@ > ? ? ? ? ? ? channel_layout = enc->channel_layout; > ? ? ? ? ? ? audio_channels = enc->channels; > ? ? ? ? ? ? audio_sample_rate = enc->sample_rate; > + ? ? ? ? ? ?audio_bit_rate = enc->bit_rate; > ? ? ? ? ? ? audio_sample_fmt = enc->sample_fmt; > ? ? ? ? ? ? input_codecs[nb_icodecs++] = > avcodec_find_decoder_by_name(audio_codec_name); > ? ? ? ? ? ? if(audio_disable) > @@ -3211,6 +3216,7 @@ > ? ? } > ? ? nb_ocodecs++; > ? ? audio_enc->sample_rate = audio_sample_rate; > + ? ?audio_enc->bit_rate = audio_bit_rate; > ? ? audio_enc->time_base= (AVRational){1, audio_sample_rate}; > ? ? if (audio_language) { > ? ? ? ? av_metadata_set(&st->metadata, "language", audio_language);
> Index: libavformat/avformat.h > =================================================================== > --- libavformat/avformat.h ?(revision 18096) > +++ libavformat/avformat.h ?(working copy) > @@ -261,6 +261,7 @@ > ? ? enum CodecID video_codec_id; > ? ? enum CodecID audio_codec_id; > #endif > + ? ?int bit_rate; > } AVFormatParameters; > > //! Demuxer will use url_fopen, no opened file should be provided by the > caller.
the whole struct is pretty much deprecated and iam not happy if people add new things to it.
any hint on to "do it the right way" ?
AVFormatContext.bit_rate maybe but i am not sure if this might cause some issues
actually a much simpler solution would be to change the -ab options to set in the AVCodecContext.bitrate for decoder too like in the patch attached
but it would change the behavior of ffmpeg
but that's exactly what is needed, to be able to give the g722 decoder a bit_rate
would that be acceptable?
have you checked that this doesnt break transcoding with multiple inputs and outputs, i mean the -ab must not mistakely override the wrong one
I tried with different samples and so far it works, but I am still working on making sure no regression happens by inspecting the audio decoder.
In the meantime, the init function overwrites the AVCodecContext.bit_rate to the default g722 value of 64000 and issue a warning if the standard compliance is not set to strict.
new patches attached [...] +/** + * adaptive predictor + * + * @note On x86 using the MULL macro in a loop is slower than not using the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_part_reconst; + + band->qtzd_reconst_mem[1] = band->qtzd_reconst_mem[0]; + band->qtzd_reconst_mem[0] = av_clip_int16((band->s_predictor + cur_diff) << 1); + + cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]];
i dont see why a LUT should be used here, its not really more readable and i doubt its faster.
+ band->part_reconst_mem[1] = band->part_reconst_mem[0]; + band->part_reconst_mem[0] = cur_part_reconst; + + band->pole_mem[1] = av_clip((sg[0] * av_clip(band->pole_mem[0], -8191, 8191) >> 5) + + (sg[1] << 7) + MULL(band->pole_mem[1], 127, 7), -12288, 12288); + + limit = 15360 - band->pole_mem[1]; + band->pole_mem[0] = av_clip(-192 * sg[0] + MULL(band->pole_mem[0], 255, 8), -limit, limit); + + + if(cur_diff) { + for (i = 0; i < 6; i++) + band->zero_mem[i] = ((band->zero_mem[i]*255) >> 8) +
+ (band->diff_mem[i] >> 15 == cur_diff >> 15 ? 128 : -128);
if this is testing the equality of the sign bit then (x^y) < 0 might be faster [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Apr 3, 2009, at 9:42 PM, Michael Niedermayer wrote:
> On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
>> Hi
>>
>> On Thu, Mar 26, 2009 at 11:24 AM, Michael Niedermayer <michaelni at gmx.at
>> > wrote:
>>> On Wed, Mar 25, 2009 at 09:37:58PM -0700, Kenan Gillet wrote:
>>>> On Wed, Mar 25, 2009 at 2:39 PM, Michael Niedermayer <michaelni at gmx.at
>>>> > wrote:
>>>>> On Wed, Mar 25, 2009 at 09:57:51AM -0700, Kenan Gillet wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Mar 21, 2009, at 1:34 PM, Michael Niedermayer wrote:
>>>>>>
>>>>>>> On Sat, Mar 21, 2009 at 12:43:35AM -0700, Kenan Gillet wrote:
>>>>> [...]
>>>>>>>
>>>>>>>
>>>>>>>> @@ -2819,6 +2822,7 @@
>>>>>>>> memset(ap, 0, sizeof(*ap));
>>>>>>>> ap->prealloced_context = 1;
>>>>>>>> ap->sample_rate = audio_sample_rate;
>>>>>>>> + ap->bit_rate = audio_bit_rate;
>>>>>>>> ap->channels = audio_channels;
>>>>>>>> ap->time_base.den = frame_rate.num;
>>>>>>>> ap->time_base.num = frame_rate.den;
>>>>>>>> @@ -2892,6 +2896,7 @@
>>>>>>>> channel_layout = enc->channel_layout;
>>>>>>>> audio_channels = enc->channels;
>>>>>>>> audio_sample_rate = enc->sample_rate;
>>>>>>>> + audio_bit_rate = enc->bit_rate;
>>>>>>>> audio_sample_fmt = enc->sample_fmt;
>>>>>>>> input_codecs[nb_icodecs++] =
>>>>>>>> avcodec_find_decoder_by_name(audio_codec_name);
>>>>>>>> if(audio_disable)
>>>>>>>> @@ -3211,6 +3216,7 @@
>>>>>>>> }
>>>>>>>> nb_ocodecs++;
>>>>>>>> audio_enc->sample_rate = audio_sample_rate;
>>>>>>>> + audio_enc->bit_rate = audio_bit_rate;
>>>>>>>> audio_enc->time_base= (AVRational){1, audio_sample_rate};
>>>>>>>> if (audio_language) {
>>>>>>>> av_metadata_set(&st->metadata, "language",
>>>>>>>> audio_language);
>>>>>>>
>>>>>>>> Index: libavformat/avformat.h
>>>>>>>> =
>>>>>>>> =
>>>>>>>> =
>>>>>>>> =
>>>>>>>> ===============================================================
>>>>>>>> --- libavformat/avformat.h (revision 18096)
>>>>>>>> +++ libavformat/avformat.h (working copy)
>>>>>>>> @@ -261,6 +261,7 @@
>>>>>>>> enum CodecID video_codec_id;
>>>>>>>> enum CodecID audio_codec_id;
>>>>>>>> #endif
>>>>>>>> + int bit_rate;
>>>>>>>> } AVFormatParameters;
>>>>>>>>
>>>>>>>> //! Demuxer will use url_fopen, no opened file should be
>>>>>>>> provided by the
>>>>>>>> caller.
>>>>>>>
>>>>>>> the whole struct is pretty much deprecated and iam not happy
>>>>>>> if people
>>>>>>> add new things to it.
>>>>>>
>>>>>> any hint on to "do it the right way" ?
>>>>>
>>>>> AVFormatContext.bit_rate maybe but i am not sure if this might
>>>>> cause some
>>>>> issues
>>>>>
>>>>
>>>> actually a much simpler solution would be to change the -ab
>>>> options to
>>>> set in the
>>>> AVCodecContext.bitrate for decoder too like in the patch attached
>>>>
>>>> but it would change the behavior of ffmpeg
>>>>
>>>> but that's exactly what is needed, to be able to give the g722
>>>> decoder
>>>> a bit_rate
>>>>
>>>> would that be acceptable?
>>>
>>> have you checked that this doesnt break transcoding with multiple
>>> inputs and
>>> outputs, i mean the -ab must not mistakely override the wrong one
>>
>> I tried with different samples and so far it works, but I am still
>> working on making
>> sure no regression happens by inspecting the audio decoder.
>>
>> In the meantime, the init function overwrites the
>> AVCodecContext.bit_rate to the
>> default g722 value of 64000 and issue a warning if the standard
>> compliance is not
>> set to strict.
>>
>> new patches attached
> [...]
>> +/**
>> + * adaptive predictor
>> + *
>> + * @note On x86 using the MULL macro in a loop is slower than not
>> using the macro.
>> + */
>> +static void do_adaptive_prediction(struct G722Band *band, const
>> int cur_diff)
>> +{
>> + int sg[2], limit, i, cur_part_reconst;
>> +
>> + band->qtzd_reconst_mem[1] = band->qtzd_reconst_mem[0];
>> + band->qtzd_reconst_mem[0] = av_clip_int16((band->s_predictor +
>> cur_diff) << 1);
>> +
>> + cur_part_reconst = band->s_zero + cur_diff < 0;
>> +
>> + sg[0] = sign_lookup[cur_part_reconst != band-
>> >part_reconst_mem[0]];
>> + sg[1] = sign_lookup[cur_part_reconst == band-
>> >part_reconst_mem[1]];
>
> i dont see why a LUT should be used here, its not really more readable
> and i doubt its faster.
it is faster
on Core 2 Duo 2Ghz , gcc 4.2.1
LUT based:
testing for 64kb 16KHz: [OK][ 1574 dezicycles in
do_adaptive_prediction, 262130 runs, 14 skips ]
testing for 56kb 16KHz: [OK][ 1668 dezicycles in
do_adaptive_prediction, 262110 runs, 34 skips ]
testing for 48kb 16KHz: [OK][ 1607 dezicycles in
do_adaptive_prediction, 262112 runs, 32 skips ]
testing for 64Kb 8KHz: [OK][ 1584 dezicycles in
do_adaptive_prediction, 131066 runs, 6 skips ]
testing encoding for 64Kb 16KHz: [OK][ 1558 dezicycles in
do_adaptive_prediction, 262108 runs, 36 skips ]
non-LUT:
testing for 64kb 16KHz: [OK][ 1686 dezicycles in
do_adaptive_prediction, 262126 runs, 18 skips ]
testing for 56kb 16KHz: [OK][ 1719 dezicycles in
do_adaptive_prediction, 262107 runs, 37 skips ]
testing for 48kb 16KHz: [OK][ 1689 dezicycles in
do_adaptive_prediction, 262126 runs, 18 skips ]
testing for 64Kb 8KHz: [OK][ 1676 dezicycles in
do_adaptive_prediction, 131065 runs, 7 skips ]
testing encoding for 64Kb 16KHz: [OK][ 1673 dezicycles in
do_adaptive_prediction, 262116 runs, 28 skips ]
i can revert to non-LUT if you prefer
>> + band->part_reconst_mem[1] = band->part_reconst_mem[0];
>> + band->part_reconst_mem[0] = cur_part_reconst;
>> +
>> + band->pole_mem[1] = av_clip((sg[0] * av_clip(band-
>> >pole_mem[0], -8191, 8191) >> 5) +
>> + (sg[1] << 7) + MULL(band-
>> >pole_mem[1], 127, 7), -12288, 12288);
>> +
>> + limit = 15360 - band->pole_mem[1];
>> + band->pole_mem[0] = av_clip(-192 * sg[0] + MULL(band-
>> >pole_mem[0], 255, 8), -limit, limit);
>> +
>> +
>> + if(cur_diff) {
>> + for (i = 0; i < 6; i++)
>> + band->zero_mem[i] = ((band->zero_mem[i]*255) >> 8) +
>
>> + (band->diff_mem[i] >> 15 ==
>> cur_diff >> 15 ? 128 : -128);
>
> if this is testing the equality of the sign bit then
> (x^y) < 0
> might be faster
yes it is faster :)
before
testing for 64kb 16KHz: [OK][ 1651 dezicycles in
do_adaptive_prediction, 262104 runs, 40 skips ]
testing for 56kb 16KHz: [OK][ 1770 dezicycles in
do_adaptive_prediction, 262117 runs, 27 skips ]
testing for 48kb 16KHz: [OK][ 1683 dezicycles in
do_adaptive_prediction, 262118 runs, 26 skips ]
testing for 64Kb 8KHz: [OK][ 1660 dezicycles in
do_adaptive_prediction, 131064 runs, 8 skips ]
testing encoding for 64Kb 16KHz: [OK][ 1637 dezicycles in
do_adaptive_prediction, 262110 runs, 34 skips ]
after:
testing for 64kb 16KHz: [OK][ 1575 dezicycles in
do_adaptive_prediction, 262114 runs, 30 skips ]
testing for 56kb 16KHz: [OK][ 1668 dezicycles in
do_adaptive_prediction, 262116 runs, 28 skips ]
testing for 48kb 16KHz: [OK][ 1607 dezicycles in
do_adaptive_prediction, 262123 runs, 21 skips ]
testing for 64Kb 8KHz: [OK][ 1585 dezicycles in
do_adaptive_prediction, 131061 runs, 11 skips ]
testing encoding for 64Kb 16KHz: [OK][ 1558 dezicycles in
do_adaptive_prediction, 262124 runs, 20 skips ]
I also forgot to mention that the encoder and decoder passes the
official ITU test sequences
at http://www.itu.int/rec/T-REC-G.722-198703-I!AppII/en
thanks for the review
Kenan
Hi
On Apr 4, 2009, at 11:46 AM, Kenan Gillet wrote:
>
> On Apr 3, 2009, at 9:42 PM, Michael Niedermayer wrote:
>
>> On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
[...]
>>> +/**
>>> + * adaptive predictor
>>> + *
>>> + * @note On x86 using the MULL macro in a loop is slower than not
>>> using the macro.
>>> + */
>>> +static void do_adaptive_prediction(struct G722Band *band, const
>>> int cur_diff)
>>> +{
>>> + int sg[2], limit, i, cur_part_reconst;
>>> +
>>> + band->qtzd_reconst_mem[1] = band->qtzd_reconst_mem[0];
>>> + band->qtzd_reconst_mem[0] = av_clip_int16((band->s_predictor
>>> + cur_diff) << 1);
>>> +
>>> + cur_part_reconst = band->s_zero + cur_diff < 0;
>>> +
>>> + sg[0] = sign_lookup[cur_part_reconst != band-
>>> >part_reconst_mem[0]];
>>> + sg[1] = sign_lookup[cur_part_reconst == band-
>>> >part_reconst_mem[1]];
>>
>> i dont see why a LUT should be used here, its not really more
>> readable
>> and i doubt its faster.
>
> it is faster
>
> on Core 2 Duo 2Ghz , gcc 4.2.1
> LUT based:
> testing for 64kb 16KHz: [OK][ 1574 dezicycles in
> do_adaptive_prediction, 262130 runs, 14 skips ]
> testing for 56kb 16KHz: [OK][ 1668 dezicycles in
> do_adaptive_prediction, 262110 runs, 34 skips ]
> testing for 48kb 16KHz: [OK][ 1607 dezicycles in
> do_adaptive_prediction, 262112 runs, 32 skips ]
> testing for 64Kb 8KHz: [OK][ 1584 dezicycles in
> do_adaptive_prediction, 131066 runs, 6 skips ]
> testing encoding for 64Kb 16KHz: [OK][ 1558 dezicycles in
> do_adaptive_prediction, 262108 runs, 36 skips ]
>
>
> non-LUT:
> testing for 64kb 16KHz: [OK][ 1686 dezicycles in
> do_adaptive_prediction, 262126 runs, 18 skips ]
> testing for 56kb 16KHz: [OK][ 1719 dezicycles in
> do_adaptive_prediction, 262107 runs, 37 skips ]
> testing for 48kb 16KHz: [OK][ 1689 dezicycles in
> do_adaptive_prediction, 262126 runs, 18 skips ]
> testing for 64Kb 8KHz: [OK][ 1676 dezicycles in
> do_adaptive_prediction, 131065 runs, 7 skips ]
> testing encoding for 64Kb 16KHz: [OK][ 1673 dezicycles in
> do_adaptive_prediction, 262116 runs, 28 skips ]
>
> i can revert to non-LUT if you prefer
sorry for the noise, it though the comment was about using the sg
array not just the lookup table.
remove the use of the lookup table locally
Kenan
On Apr 4, 2009, at 12:06 PM, Kenan Gillet wrote:
> Hi
> On Apr 4, 2009, at 11:46 AM, Kenan Gillet wrote:
>>
>> On Apr 3, 2009, at 9:42 PM, Michael Niedermayer wrote:
>>
>>> On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
>
> [...]
>>>> +/**
>>>> + * adaptive predictor
>>>> + *
>>>> + * @note On x86 using the MULL macro in a loop is slower than
>>>> not using the macro.
>>>> + */
>>>> +static void do_adaptive_prediction(struct G722Band *band, const
>>>> int cur_diff)
>>>> +{
>>>> + int sg[2], limit, i, cur_part_reconst;
>>>> +
>>>> + band->qtzd_reconst_mem[1] = band->qtzd_reconst_mem[0];
>>>> + band->qtzd_reconst_mem[0] = av_clip_int16((band->s_predictor
>>>> + cur_diff) << 1);
>>>> +
>>>> + cur_part_reconst = band->s_zero + cur_diff < 0;
>>>> +
>>>> + sg[0] = sign_lookup[cur_part_reconst != band-
>>>> >part_reconst_mem[0]];
>>>> + sg[1] = sign_lookup[cur_part_reconst == band-
>>>> >part_reconst_mem[1]];
>>>
>>> i dont see why a LUT should be used here, its not really more
>>> readable
>>> and i doubt its faster.
>>
>> it is faster
>>
>> on Core 2 Duo 2Ghz , gcc 4.2.1
>> LUT based:
>> testing for 64kb 16KHz: [OK][ 1574 dezicycles in
>> do_adaptive_prediction, 262130 runs, 14 skips ]
>> testing for 56kb 16KHz: [OK][ 1668 dezicycles in
>> do_adaptive_prediction, 262110 runs, 34 skips ]
>> testing for 48kb 16KHz: [OK][ 1607 dezicycles in
>> do_adaptive_prediction, 262112 runs, 32 skips ]
>> testing for 64Kb 8KHz: [OK][ 1584 dezicycles in
>> do_adaptive_prediction, 131066 runs, 6 skips ]
>> testing encoding for 64Kb 16KHz: [OK][ 1558 dezicycles in
>> do_adaptive_prediction, 262108 runs, 36 skips ]
>>
>>
>> non-LUT:
>> testing for 64kb 16KHz: [OK][ 1686 dezicycles in
>> do_adaptive_prediction, 262126 runs, 18 skips ]
>> testing for 56kb 16KHz: [OK][ 1719 dezicycles in
>> do_adaptive_prediction, 262107 runs, 37 skips ]
>> testing for 48kb 16KHz: [OK][ 1689 dezicycles in
>> do_adaptive_prediction, 262126 runs, 18 skips ]
>> testing for 64Kb 8KHz: [OK][ 1676 dezicycles in
>> do_adaptive_prediction, 131065 runs, 7 skips ]
>> testing encoding for 64Kb 16KHz: [OK][ 1673 dezicycles in
>> do_adaptive_prediction, 262116 runs, 28 skips ]
>>
>> i can revert to non-LUT if you prefer
>
> sorry for the noise, it though the comment was about using the sg
> array not just the lookup table.
> remove the use of the lookup table locally
I knew I should not have answered before benchmarking,
conter intuitive but the LUT is still faster
with LUT:
testing for 64kb 16KHz: [OK][ 1575 dezicycles in
do_adaptive_prediction, 262113 runs, 31 skips ]
testing for 56kb 16KHz: [OK][ 1663 dezicycles in
do_adaptive_prediction, 262131 runs, 13 skips ]
testing for 48kb 16KHz: [OK][ 1608 dezicycles in
do_adaptive_prediction, 262117 runs, 27 skips ]
testing for 64Kb 8KHz: [OK][ 1583 dezicycles in
do_adaptive_prediction, 131066 runs, 6 skips ]
testing encoding for 64Kb 16KHz: [OK][ 1558 dezicycles in
do_adaptive_prediction, 262117 runs, 27 skips ]
without LUT:
testing for 64kb 16KHz: [OK][ 1672 dezicycles in
do_adaptive_prediction, 262138 runs, 6 skips ]
testing for 56kb 16KHz: [OK][ 1713 dezicycles in
do_adaptive_prediction, 262087 runs, 57 skips ]
testing for 48kb 16KHz: [OK][ 1685 dezicycles in
do_adaptive_prediction, 262125 runs, 19 skips ]
testing for 64Kb 8KHz: [OK][ 1683 dezicycles in
do_adaptive_prediction, 131065 runs, 7 skips ]
testing encoding for 64Kb 16KHz: [OK][ 1652 dezicycles in
do_adaptive_prediction, 262138 runs, 6 skips ]
Kenan
On Sat, Apr 04, 2009 at 12:39:46PM -0700, Kenan Gillet wrote:
On Apr 4, 2009, at 12:06 PM, Kenan Gillet wrote:
Hi On Apr 4, 2009, at 11:46 AM, Kenan Gillet wrote:
On Apr 3, 2009, at 9:42 PM, Michael Niedermayer wrote:
On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
[...]
+/** + * adaptive predictor + * + * @note On x86 using the MULL macro in a loop is slower than not using the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_part_reconst; + + band->qtzd_reconst_mem[1] = band->qtzd_reconst_mem[0]; + band->qtzd_reconst_mem[0] = av_clip_int16((band->s_predictor + cur_diff) << 1); + + cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]];
i dont see why a LUT should be used here, its not really more readable and i doubt its faster.
it is faster
on Core 2 Duo 2Ghz , gcc 4.2.1 LUT based: testing for 64kb 16KHz: [OK][ 1574 dezicycles in do_adaptive_prediction, 262130 runs, 14 skips ] testing for 56kb 16KHz: [OK][ 1668 dezicycles in do_adaptive_prediction, 262110 runs, 34 skips ] testing for 48kb 16KHz: [OK][ 1607 dezicycles in do_adaptive_prediction, 262112 runs, 32 skips ] testing for 64Kb 8KHz: [OK][ 1584 dezicycles in do_adaptive_prediction, 131066 runs, 6 skips ] testing encoding for 64Kb 16KHz: [OK][ 1558 dezicycles in do_adaptive_prediction, 262108 runs, 36 skips ]
non-LUT: testing for 64kb 16KHz: [OK][ 1686 dezicycles in do_adaptive_prediction, 262126 runs, 18 skips ] testing for 56kb 16KHz: [OK][ 1719 dezicycles in do_adaptive_prediction, 262107 runs, 37 skips ] testing for 48kb 16KHz: [OK][ 1689 dezicycles in do_adaptive_prediction, 262126 runs, 18 skips ] testing for 64Kb 8KHz: [OK][ 1676 dezicycles in do_adaptive_prediction, 131065 runs, 7 skips ] testing encoding for 64Kb 16KHz: [OK][ 1673 dezicycles in do_adaptive_prediction, 262116 runs, 28 skips ]
i can revert to non-LUT if you prefer
sorry for the noise, it though the comment was about using the sg array not just the lookup table. remove the use of the lookup table locally
I knew I should not have answered before benchmarking, conter intuitive but the LUT is still faster
what code did you use for the non LUT variant? 1 | - (a==b) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle
Hi On Apr 4, 2009, at 1:46 PM, Michael Niedermayer wrote:
On Sat, Apr 04, 2009 at 12:39:46PM -0700, Kenan Gillet wrote:
On Apr 4, 2009, at 12:06 PM, Kenan Gillet wrote:
Hi On Apr 4, 2009, at 11:46 AM, Kenan Gillet wrote:
On Apr 3, 2009, at 9:42 PM, Michael Niedermayer wrote:
On Tue, Mar 31, 2009 at 11:34:34PM -0700, Kenan Gillet wrote:
[...]
+/** + * adaptive predictor + * + * @note On x86 using the MULL macro in a loop is slower than not using the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_part_reconst; + + band->qtzd_reconst_mem[1] = band->qtzd_reconst_mem[0]; + band->qtzd_reconst_mem[0] = av_clip_int16((band- >s_predictor + cur_diff) << 1); + + cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]];
i dont see why a LUT should be used here, its not really more readable and i doubt its faster.
it is faster
on Core 2 Duo 2Ghz , gcc 4.2.1 LUT based: testing for 64kb 16KHz: [OK][ 1574 dezicycles in do_adaptive_prediction, 262130 runs, 14 skips ] testing for 56kb 16KHz: [OK][ 1668 dezicycles in do_adaptive_prediction, 262110 runs, 34 skips ] testing for 48kb 16KHz: [OK][ 1607 dezicycles in do_adaptive_prediction, 262112 runs, 32 skips ] testing for 64Kb 8KHz: [OK][ 1584 dezicycles in do_adaptive_prediction, 131066 runs, 6 skips ] testing encoding for 64Kb 16KHz: [OK][ 1558 dezicycles in do_adaptive_prediction, 262108 runs, 36 skips ]
non-LUT: testing for 64kb 16KHz: [OK][ 1686 dezicycles in do_adaptive_prediction, 262126 runs, 18 skips ] testing for 56kb 16KHz: [OK][ 1719 dezicycles in do_adaptive_prediction, 262107 runs, 37 skips ] testing for 48kb 16KHz: [OK][ 1689 dezicycles in do_adaptive_prediction, 262126 runs, 18 skips ] testing for 64Kb 8KHz: [OK][ 1676 dezicycles in do_adaptive_prediction, 131065 runs, 7 skips ] testing encoding for 64Kb 16KHz: [OK][ 1673 dezicycles in do_adaptive_prediction, 262116 runs, 28 skips ]
i can revert to non-LUT if you prefer
sorry for the noise, it though the comment was about using the sg array not just the lookup table. remove the use of the lookup table locally
I knew I should not have answered before benchmarking, conter intuitive but the LUT is still faster
what code did you use for the non LUT variant?
1 | - (a==b)
i used a==b? -1 : 1 but LUT is still faster 1 | - (a==b) sg[0] = 1 | -(cur_part_reconst == band->part_reconst_mem[0]); sg[1] = 1 | -(cur_part_reconst != band->part_reconst_mem[1]); testing for 64kb 16KHz: [OK][ 1463 dezicycles in do_adaptive_prediction, 262126 runs, 18 skips ] testing for 56kb 16KHz: [OK][ 1550 dezicycles in do_adaptive_prediction, 262116 runs, 28 skips ] testing for 48kb 16KHz: [OK][ 1502 dezicycles in do_adaptive_prediction, 262111 runs, 33 skips ] testing for 64Kb 8KHz: [OK][ 1474 dezicycles in do_adaptive_prediction, 131065 runs, 7 skips ] testing encoding for 64Kb 16KHz: [OK][ 1447 dezicycles in do_adaptive_prediction, 262108 runs, 36 skips ] LUT sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]]; testing for 64kb 16KHz: [OK][ 1443 dezicycles in do_adaptive_prediction, 262110 runs, 34 skips ] testing for 56kb 16KHz: [OK][ 1519 dezicycles in do_adaptive_prediction, 262121 runs, 23 skips ] testing for 48kb 16KHz: [OK][ 1477 dezicycles in do_adaptive_prediction, 262100 runs, 44 skips ] testing for 64Kb 8KHz: [OK][ 1454 dezicycles in do_adaptive_prediction, 131058 runs, 14 skips ] testing encoding for 64Kb 16KHz: [OK][ 1427 dezicycles in do_adaptive_prediction, 262123 runs, 21 skips ] BTW thanks for your check sign trick, it allowed some nice simplifications and some speedup :) Kenan
Hi, here is a second version of the g722 decoder patch. It contains some simplification and optimizations: - simplification of G722Band.s_zero calculation - only keep 1 needed quantized reconstructed value in G722Band instead of 2. thanks for the review Kenan
On Sat, Apr 04, 2009 at 03:40:13PM -0700, Kenan Gillet wrote:
--- libavcodec/g722.c (revision 0) +++ libavcodec/g722.c (revision 0) @@ -0,0 +1,349 @@ + +/** + * @file libavcodec/g722dec.c + * + * G.722 ADPCM audio codec + * + * This G.722 decoder is a bit exact implementation of the ITU G.722 specification
bit-exact
+ * for all three specified bit rates - 64000bps, 56000bps and 48000bps.
bitrates
+/** + * quadrature mirror filters (QMF) coefficients
filter
+ if(cur_diff) {
if (
+static void update_low_predictor(struct G722Band *band, const int ilow) {
Please consistently use K&R function declarations, i.e. place the { on the next line, same below.
+static int g722_encode_frame(AVCodecContext *avctx, + uint8_t *dst, int buf_size, void *data)
The indentation is off.
+ for (j = 0; j < buf_size; ) {
stray space before (
+#endif \ No newline at end of file
Fix this. Documentation and build system parts are OK. Diego
On Apr 5, 2009, at 3:37 AM, Diego Biurrun wrote:
On Sat, Apr 04, 2009 at 03:40:13PM -0700, Kenan Gillet wrote:
--- libavcodec/g722.c (revision 0) +++ libavcodec/g722.c (revision 0) @@ -0,0 +1,349 @@ + +/** + * @file libavcodec/g722dec.c + * + * G.722 ADPCM audio codec + * + * This G.722 decoder is a bit exact implementation of the ITU G. 722 specification
bit-exact
done
+ * for all three specified bit rates - 64000bps, 56000bps and 48000bps.
bitrates
done
+/** + * quadrature mirror filters (QMF) coefficients
filter
done
+ if(cur_diff) {
if (
done
+static void update_low_predictor(struct G722Band *band, const int ilow) {
Please consistently use K&R function declarations, i.e. place the { on the next line, same below.
done
+static int g722_encode_frame(AVCodecContext *avctx, + uint8_t *dst, int buf_size, void *data)
The indentation is off.
done
+ for (j = 0; j < buf_size; ) {
stray space before (
done
+#endif \ No newline at end of file
Fix this.
Documentation and build system parts are OK.
done thanks for the review Kenan
Hi, here is a version 3 of the patch: - it includes the Diego's suggested changes - g722_decode_frame use the new API thanks for the reviews Kenan
On Wed, Apr 08, 2009 at 09:23:51PM -0700, Kenan Gillet wrote:
here is a version 3 of the patch: - it includes the Diego's suggested changes
It does not. Diego
On Thu, Apr 9, 2009 at 12:12 AM, Diego Biurrun <diego at biurrun.de> wrote:
On Wed, Apr 08, 2009 at 09:23:51PM -0700, Kenan Gillet wrote:
here is a version 3 of the patch: - it includes the Diego's suggested changes
It does not.
darn, i double checked the changes, and just ended up attaching the wrong file :( anyway correct patch attached, thanks Diego Kenan
On Thu, Apr 09, 2009 at 08:14:58AM -0700, Kenan Gillet wrote:
On Thu, Apr 9, 2009 at 12:12 AM, Diego Biurrun <diego at biurrun.de> wrote:
On Wed, Apr 08, 2009 at 09:23:51PM -0700, Kenan Gillet wrote:
here is a version 3 of the patch: - it includes the Diego's suggested changes
It does not.
darn, i double checked the changes, and just ended up attaching the wrong file :(
anyway correct patch attached, [...] + * @note For the 56000bps and 48000bps bitrates, the respectively 7 and 6 bits + * codeword might be packed, so unpacking might be needed either + * internally or as a separate parser.
do non packed cases exist anywhere in the wild? do packed cases exist anywhere in the wild? it seems to me that filling 30% of the bits with 0 is not particlarely likely in a real world environment
+ */ + +#include <stdint.h> +#include "avcodec.h" +#include "mathops.h" + +typedef struct { + int bits_per_sample; ///< 6 for 48000kbps, 7 for 56000kbps, or 8 for 64000kbps + + int16_t prev_samples[24]; ///< memory of past 24 received (decoded) samples + + /** + * The band[0] and band[1] correspond respectively to the lower band and higher band. + */ + struct G722Band {
+ int16_t s_predictor; ///< predictor output value
ok that makes sense
+ int32_t s_zero; ///< zero section output signal
what is that?
+ int8_t part_reconst_mem[2]; ///< partially reconstructed signal memory
and that?
+ int16_t prev_qtzd_reconst; ///< previous quantized reconstructed signal
this is not true, this uses a different dequantization table in some cases i even wonder if this is a bug
+ int16_t pole_mem[2]; ///< second-order pole section coefficient buffer + int32_t diff_mem[6]; ///< quantizer difference signal memory + int16_t zero_mem[6]; ///< Seventh-order zero section coefficient buffer
+ int16_t log_factor; ///< delayed logarithmic quantizer factor
log what? log_e log_2 log_10 ?
+ int16_t scale_factor; ///< delayed quantizer scale factor + } band[2]; +} G722Context; + + +static const int8_t sign_lookup[2] = { -1, 1 }; + +static const int16_t ilb[32] = { + 2048, 2093, 2139, 2186, 2233, 2282, 2332, 2383, + 2435, 2489, 2543, 2599, 2656, 2714, 2774, 2834, + 2896, 2960, 3025, 3091, 3158, 3228, 3298, 3371, + 3444, 3520, 3597, 3676, 3756, 3838, 3922, 4008 +};
+static const int16_t wh[2] = { 798, -214 };
2 letters is better than 1 ... well sometimes its not much better though
+static const int16_t qm2[4] = { -7408, -1616, 7408, 1616 };
+/** + * qm3[index] == wl[rl42[index]] + */ +static const int16_t qm3[16] = { + -60, 3042, 1198, 538, 334, 172, 58, -30, + 3042, 1198, 538, 334, 172, 58, -30, -60 +}; +static const int16_t qm4[16] = { + 0, -20456, -12896, -8968, -6288, -4240, -2584, -1200, + 20456, 12896, 8968, 6288, 4240, 2584, 1200, 0 +};
what is qm? i know these are dequantization tables, and at that poorly designed ones (yeah the duplicate entries) but i cant relate "qm" to "dequantization"
+ +/** + * quadrature mirror filter (QMF) coefficients + * + * ITU-T G.722 Table 11 + */ +static const int16_t qmf_coeffs[12] = { + 3, -11, 12, 32, -210, 951, 3876, -805, 362, -156, 53, -11, +}; + +
+/** + * adaptive predictor + * + * @note On x86 using the MULL macro in a loop is slower than not using the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff)
missing doxy for cur_diff [...]
+static int inline scale(const int log_factor, int shift) +{ + const int wd1 = ilb[(log_factor >> 6) & 31]; + shift -= log_factor >> 11; + return (shift < 0 ? wd1 << -shift : wd1 >> shift) << 2; +}
i belive we already have some integer exp2() also this one lacks documentation and a name that is related to what it does if there is a reason why common code cannot be used [...]
+static void apply_qmf(int16_t *prev_samples, int *xout1, int *xout2) +{ + int i; + + *xout1 = 0; + *xout2 = 0; + for (i = 0; i < 12; i++) { + MAC16(*xout2, prev_samples[2*i ], qmf_coeffs[i ]); + MAC16(*xout1, prev_samples[2*i+1], qmf_coeffs[11-i]); + }
+ memmove(prev_samples, prev_samples + 2, 22*sizeof(prev_samples[0]));
please find a solution that does not need to move the array by 2 samples after each 2 samples. [...]
+static int g722_decode_frame(AVCodecContext *avctx, void *data, int *data_size, AVPacket *avpkt) +{ + G722Context *c = avctx->priv_data; + int16_t *out_buf = data; + const uint8_t *buf = avpkt->data; + int j, out_len = 0; + const int shift = 8 - c->bits_per_sample; + const int16_t *quantizer_table = qms[shift]; + + for (j = 0; j < avpkt->size; j++) { + const int ilow = buf[j] & (0x3F >> shift); + const int rlow = av_clip(MULL(c->band[0].scale_factor, quantizer_table[ilow], 15) + + c->band[0].s_predictor, -16384, 16383); + + update_low_predictor(&c->band[0], ilow >> (2 - shift)); + + if (avctx->sample_rate == 16000) { + const int ihigh = (buf[j] >> (6 - shift)) & 0x03;
all that looks a litte obfuscated, get_bits() seems like the proper choice [...]
+static int g722_encode_frame(AVCodecContext *avctx, + uint8_t *dst, int buf_size, void *data) +{ + G722Context *c = avctx->priv_data; + const int16_t *samples = data; + + int diff, limit; + int i, j; + + for (j = 0; j < buf_size; ) { + int xlow, rlow; + if (avctx->sample_rate == 16000) { + int xout1, xout2, xhigh, diff, pred, index; + + c->prev_samples[22] = samples[j++]; + c->prev_samples[23] = samples[j++]; + apply_qmf(c->prev_samples, &xout1, &xout2); + xlow = (xout1 + xout2) >> 14; + xhigh = (xout1 - xout2) >> 14; + + diff = av_clip_int16(xhigh - c->band[1].s_predictor); + pred = MULL(564, c->band[1].scale_factor, 12); + index = diff >= 0 ? (diff < pred) + 2 + : diff >= -pred; + + update_high_predictor(&c->band[1], MULL(c->band[1].scale_factor, qm2[index], 15), index); + + *dst = (index << 6); + } else + xlow = samples[j++]; + + diff = av_clip_int16(xlow - c->band[0].s_predictor); + limit = diff >= 0 ? diff : -(diff + 1); + for (i = 0; i < 29 && limit >= (q6[i]*c->band[0].scale_factor) >> 12; i++) + ; + *dst |= + rlow = (diff < 0 ? (i < 2 ? 63 : 33) : 61) - i;
i have good news and bad news the good are that quality might be significatly improvable by using trellis/viterbi, the bad news is that the encoder is rejected unless you either implement that or proof that it cant be done (and i dont mean full viterbi but keeping more than 1 possible previous bitstream that is not just 1 previous and the best current quantized value but several best past ones X several current quantized values ...) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Fri, Apr 10, 2009 at 05:22:48AM +0200, Michael Niedermayer wrote:
i have good news and bad news the good are that quality might be significatly improvable by using trellis/viterbi, the bad news is that the encoder is rejected unless you either implement that or proof that it cant be done (and i dont mean full viterbi but keeping more than 1 possible previous bitstream that is not just 1 previous and the best current quantized value but several best past ones X several current quantized values ...)
Kenan, submit the decoder first, you can worry about the encoder later. Diego
On Apr 9, 2009, at 10:01 PM, Diego Biurrun wrote:
On Fri, Apr 10, 2009 at 05:22:48AM +0200, Michael Niedermayer wrote:
i have good news and bad news the good are that quality might be significatly improvable by using trellis/viterbi, the bad news is that the encoder is rejected unless you either implement that or proof that it cant be done (and i dont mean full viterbi but keeping more than 1 possible previous bitstream that is not just 1 previous and the best current quantized value but several best past ones X several current quantized values ...)
Kenan, submit the decoder first, you can worry about the encoder later.
thanks Diego, I'll do that Kenan
Hi, The G.722 decoder patch that Kenan Gillet took a far way through the review process last year seemed to have stalled, I'm trying to pick it up and get it committed. On Fri, 10 Apr 2009, Michael Niedermayer wrote:
On Thu, Apr 09, 2009 at 08:14:58AM -0700, Kenan Gillet wrote: [...]
+ * @note For the 56000bps and 48000bps bitrates, the respectively 7 and 6 bits + * codeword might be packed, so unpacking might be needed either + * internally or as a separate parser.
do non packed cases exist anywhere in the wild? do packed cases exist anywhere in the wild?
it seems to me that filling 30% of the bits with 0 is not particlarely likely in a real world environment
According to the spec, the point of the lower bitrate versions is to be able to use 1 or 2 bits per byte as an auxillary data channel. The encoder always outputs the 64 kbps mode, but you can choose to overwrite the lowest two bits if you want to. The decoder is able to cope with these bits being overwritten with random bits, but you get slightly better results if you tell the decoder that this has happened and so it should disregard those bits. With that in mind, as far as I can interpret the spec, it's the lowest bits that should be skipped for those modes, but the source for this decoder seems to think otherwise. The implementation in spandsp (that has some common heritage with this code base, iirc, does things this way, too.
+ /** + * The band[0] and band[1] correspond respectively to the lower band and higher band. + */ + struct G722Band {
+ int16_t s_predictor; ///< predictor output value
ok that makes sense
+ int32_t s_zero; ///< zero section output signal
what is that?
+ int8_t part_reconst_mem[2]; ///< partially reconstructed signal memory
and that?
Tried to clarify these doxy comments, to the best of my understanding of the spec.
+ int16_t prev_qtzd_reconst; ///< previous quantized reconstructed signal
this is not true, this uses a different dequantization table in some cases i even wonder if this is a bug
No, it's explicitly stated that it should be done this way in the spec. I figure it's part of the "scalability" feature - the predictor updating is always done using the 4-bit dequantizer so that it evolves in the same way regardless of the mode it is decoded in. Each individual output sample is calculated using the more precise dequantizer if that is chosen, but the predictor is always updated using the lower precision dequantizer.
+ int16_t pole_mem[2]; ///< second-order pole section coefficient buffer + int32_t diff_mem[6]; ///< quantizer difference signal memory + int16_t zero_mem[6]; ///< Seventh-order zero section coefficient buffer
+ int16_t log_factor; ///< delayed logarithmic quantizer factor
log what? log_e log_2 log_10 ?
log_2, clarified
+ int16_t scale_factor; ///< delayed quantizer scale factor + } band[2]; +} G722Context; + + +static const int8_t sign_lookup[2] = { -1, 1 }; + +static const int16_t ilb[32] = { + 2048, 2093, 2139, 2186, 2233, 2282, 2332, 2383, + 2435, 2489, 2543, 2599, 2656, 2714, 2774, 2834, + 2896, 2960, 3025, 3091, 3158, 3228, 3298, 3371, + 3444, 3520, 3597, 3676, 3756, 3838, 3922, 4008 +};
+static const int16_t wh[2] = { 798, -214 };
2 letters is better than 1 ... well sometimes its not much better though
Tried to give this table a better name
+static const int16_t qm2[4] = { -7408, -1616, 7408, 1616 };
+/** + * qm3[index] == wl[rl42[index]] + */ +static const int16_t qm3[16] = { + -60, 3042, 1198, 538, 334, 172, 58, -30, + 3042, 1198, 538, 334, 172, 58, -30, -60 +}; +static const int16_t qm4[16] = { + 0, -20456, -12896, -8968, -6288, -4240, -2584, -1200, + 20456, 12896, 8968, 6288, 4240, 2584, 1200, 0 +};
what is qm? i know these are dequantization tables, and at that poorly designed ones (yeah the duplicate entries) but i cant relate "qm" to "dequantization"
Tried to give better names to these tables, too
+ +/** + * quadrature mirror filter (QMF) coefficients + * + * ITU-T G.722 Table 11 + */ +static const int16_t qmf_coeffs[12] = { + 3, -11, 12, 32, -210, 951, 3876, -805, 362, -156, 53, -11, +}; + +
+/** + * adaptive predictor + * + * @note On x86 using the MULL macro in a loop is slower than not using the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff)
missing doxy for cur_diff
Added some sort of doxy for this parameter
[...]
+static int inline scale(const int log_factor, int shift) +{ + const int wd1 = ilb[(log_factor >> 6) & 31]; + shift -= log_factor >> 11; + return (shift < 0 ? wd1 << -shift : wd1 >> shift) << 2; +}
i belive we already have some integer exp2()
Didn't find any common method to do shifts where the shift can be negative - only found av_shr_i in libavutil/integer.h, that works on arbitrary precision integers.
also this one lacks documentation and a name that is related to what it does if there is a reason why common code cannot be used
Renamed it to a slightly more descriptive name
[...]
+static void apply_qmf(int16_t *prev_samples, int *xout1, int *xout2) +{ + int i; + + *xout1 = 0; + *xout2 = 0; + for (i = 0; i < 12; i++) { + MAC16(*xout2, prev_samples[2*i ], qmf_coeffs[i ]); + MAC16(*xout1, prev_samples[2*i+1], qmf_coeffs[11-i]); + }
+ memmove(prev_samples, prev_samples + 2, 22*sizeof(prev_samples[0]));
please find a solution that does not need to move the array by 2 samples after each 2 samples.
Changed to use a longer internal buffer for these samples so that memmoves are done much more seldom (every 1024 samples).
[...]
+static int g722_decode_frame(AVCodecContext *avctx, void *data, int *data_size, AVPacket *avpkt) +{ + G722Context *c = avctx->priv_data; + int16_t *out_buf = data; + const uint8_t *buf = avpkt->data; + int j, out_len = 0; + const int shift = 8 - c->bits_per_sample; + const int16_t *quantizer_table = qms[shift]; + + for (j = 0; j < avpkt->size; j++) { + const int ilow = buf[j] & (0x3F >> shift); + const int rlow = av_clip(MULL(c->band[0].scale_factor, quantizer_table[ilow], 15) + + c->band[0].s_predictor, -16384, 16383); + + update_low_predictor(&c->band[0], ilow >> (2 - shift)); + + if (avctx->sample_rate == 16000) { + const int ihigh = (buf[j] >> (6 - shift)) & 0x03;
all that looks a litte obfuscated, get_bits() seems like the proper choice
Changed to use get_bits // Martin
On Fri, 6 Aug 2010, Martin Storsj? wrote:
Hi,
The G.722 decoder patch that Kenan Gillet took a far way through the review process last year seemed to have stalled, I'm trying to pick it up and get it committed.
On Fri, 10 Apr 2009, Michael Niedermayer wrote:
On Thu, Apr 09, 2009 at 08:14:58AM -0700, Kenan Gillet wrote: [...]
+ * @note For the 56000bps and 48000bps bitrates, the respectively 7 and 6 bits + * codeword might be packed, so unpacking might be needed either + * internally or as a separate parser.
do non packed cases exist anywhere in the wild? do packed cases exist anywhere in the wild?
it seems to me that filling 30% of the bits with 0 is not particlarely likely in a real world environment
According to the spec, the point of the lower bitrate versions is to be able to use 1 or 2 bits per byte as an auxillary data channel. The encoder always outputs the 64 kbps mode, but you can choose to overwrite the lowest two bits if you want to.
The decoder is able to cope with these bits being overwritten with random bits, but you get slightly better results if you tell the decoder that this has happened and so it should disregard those bits.
With that in mind, as far as I can interpret the spec, it's the lowest bits that should be skipped for those modes, but the source for this decoder seems to think otherwise. The implementation in spandsp (that has some common heritage with this code base, iirc, does things this way, too.
+ /** + * The band[0] and band[1] correspond respectively to the lower band and higher band. + */ + struct G722Band {
+ int16_t s_predictor; ///< predictor output value
ok that makes sense
+ int32_t s_zero; ///< zero section output signal
what is that?
+ int8_t part_reconst_mem[2]; ///< partially reconstructed signal memory
and that?
Tried to clarify these doxy comments, to the best of my understanding of the spec.
+ int16_t prev_qtzd_reconst; ///< previous quantized reconstructed signal
this is not true, this uses a different dequantization table in some cases i even wonder if this is a bug
No, it's explicitly stated that it should be done this way in the spec. I figure it's part of the "scalability" feature - the predictor updating is always done using the 4-bit dequantizer so that it evolves in the same way regardless of the mode it is decoded in. Each individual output sample is calculated using the more precise dequantizer if that is chosen, but the predictor is always updated using the lower precision dequantizer.
+ int16_t pole_mem[2]; ///< second-order pole section coefficient buffer + int32_t diff_mem[6]; ///< quantizer difference signal memory + int16_t zero_mem[6]; ///< Seventh-order zero section coefficient buffer
+ int16_t log_factor; ///< delayed logarithmic quantizer factor
log what? log_e log_2 log_10 ?
log_2, clarified
+ int16_t scale_factor; ///< delayed quantizer scale factor + } band[2]; +} G722Context; + + +static const int8_t sign_lookup[2] = { -1, 1 }; + +static const int16_t ilb[32] = { + 2048, 2093, 2139, 2186, 2233, 2282, 2332, 2383, + 2435, 2489, 2543, 2599, 2656, 2714, 2774, 2834, + 2896, 2960, 3025, 3091, 3158, 3228, 3298, 3371, + 3444, 3520, 3597, 3676, 3756, 3838, 3922, 4008 +};
+static const int16_t wh[2] = { 798, -214 };
2 letters is better than 1 ... well sometimes its not much better though
Tried to give this table a better name
+static const int16_t qm2[4] = { -7408, -1616, 7408, 1616 };
+/** + * qm3[index] == wl[rl42[index]] + */ +static const int16_t qm3[16] = { + -60, 3042, 1198, 538, 334, 172, 58, -30, + 3042, 1198, 538, 334, 172, 58, -30, -60 +}; +static const int16_t qm4[16] = { + 0, -20456, -12896, -8968, -6288, -4240, -2584, -1200, + 20456, 12896, 8968, 6288, 4240, 2584, 1200, 0 +};
what is qm? i know these are dequantization tables, and at that poorly designed ones (yeah the duplicate entries) but i cant relate "qm" to "dequantization"
Tried to give better names to these tables, too
+ +/** + * quadrature mirror filter (QMF) coefficients + * + * ITU-T G.722 Table 11 + */ +static const int16_t qmf_coeffs[12] = { + 3, -11, 12, 32, -210, 951, 3876, -805, 362, -156, 53, -11, +}; + +
+/** + * adaptive predictor + * + * @note On x86 using the MULL macro in a loop is slower than not using the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff)
missing doxy for cur_diff
Added some sort of doxy for this parameter
[...]
+static int inline scale(const int log_factor, int shift) +{ + const int wd1 = ilb[(log_factor >> 6) & 31]; + shift -= log_factor >> 11; + return (shift < 0 ? wd1 << -shift : wd1 >> shift) << 2; +}
i belive we already have some integer exp2()
Didn't find any common method to do shifts where the shift can be negative - only found av_shr_i in libavutil/integer.h, that works on arbitrary precision integers.
also this one lacks documentation and a name that is related to what it does if there is a reason why common code cannot be used
Renamed it to a slightly more descriptive name
[...]
+static void apply_qmf(int16_t *prev_samples, int *xout1, int *xout2) +{ + int i; + + *xout1 = 0; + *xout2 = 0; + for (i = 0; i < 12; i++) { + MAC16(*xout2, prev_samples[2*i ], qmf_coeffs[i ]); + MAC16(*xout1, prev_samples[2*i+1], qmf_coeffs[11-i]); + }
+ memmove(prev_samples, prev_samples + 2, 22*sizeof(prev_samples[0]));
please find a solution that does not need to move the array by 2 samples after each 2 samples.
Changed to use a longer internal buffer for these samples so that memmoves are done much more seldom (every 1024 samples).
[...]
+static int g722_decode_frame(AVCodecContext *avctx, void *data, int *data_size, AVPacket *avpkt) +{ + G722Context *c = avctx->priv_data; + int16_t *out_buf = data; + const uint8_t *buf = avpkt->data; + int j, out_len = 0; + const int shift = 8 - c->bits_per_sample; + const int16_t *quantizer_table = qms[shift]; + + for (j = 0; j < avpkt->size; j++) { + const int ilow = buf[j] & (0x3F >> shift); + const int rlow = av_clip(MULL(c->band[0].scale_factor, quantizer_table[ilow], 15) + + c->band[0].s_predictor, -16384, 16383); + + update_low_predictor(&c->band[0], ilow >> (2 - shift)); + + if (avctx->sample_rate == 16000) { + const int ihigh = (buf[j] >> (6 - shift)) & 0x03;
all that looks a litte obfuscated, get_bits() seems like the proper choice
Changed to use get_bits
Ping // Martin
On Fri, Aug 06, 2010 at 09:24:23PM +0300, Martin Storsj? wrote:
Hi,
The G.722 decoder patch that Kenan Gillet took a far way through the review process last year seemed to have stalled, I'm trying to pick it up and get it committed.
On Fri, 10 Apr 2009, Michael Niedermayer wrote:
On Thu, Apr 09, 2009 at 08:14:58AM -0700, Kenan Gillet wrote: [...]
+ * @note For the 56000bps and 48000bps bitrates, the respectively 7 and 6 bits + * codeword might be packed, so unpacking might be needed either + * internally or as a separate parser.
do non packed cases exist anywhere in the wild? do packed cases exist anywhere in the wild?
it seems to me that filling 30% of the bits with 0 is not particlarely likely in a real world environment
According to the spec, the point of the lower bitrate versions is to be able to use 1 or 2 bits per byte as an auxillary data channel. The encoder always outputs the 64 kbps mode, but you can choose to overwrite the lowest two bits if you want to.
The decoder is able to cope with these bits being overwritten with random bits, but you get slightly better results if you tell the decoder that this has happened and so it should disregard those bits.
With that in mind, as far as I can interpret the spec, it's the lowest bits that should be skipped for those modes, but the source for this decoder seems to think otherwise. The implementation in spandsp (that has some common heritage with this code base, iirc, does things this way, too.
this does not awnser my question do non packed cases exist anywhere in the wild? do packed cases exist anywhere in the wild? this matters because the packed case is usefull due to lower bitrate but its not supported while the non packed case is supported in a way that you say doesnt comply to the spec and i cant see anyone using it except by mistake or "because some spec says so" [...]
+static const int16_t low_inv_quant5[32] = { + -280, -280, -23352, -17560, -14120, -11664, -9752, -8184, + -6864, -5712, -4696, -3784, -2960, -2208, -1520, -880, + 23352, 17560, 14120, 11664, 9752, 8184, 6864, 5712, + 4696, 3784, 2960, 2208, 1520, 880, 280, -280 +}; +static const int16_t low_inv_quant6[64] = { + -136, -136, -136, -136, -24808, -21904, -19008, -16704, + -14984, -13512, -12280, -11192, -10232, -9360, -8576, -7856, + -7192, -6576, -6000, -5456, -4944, -4464, -4008, -3576, + -3168, -2776, -2400, -2032, -1688, -1360, -1040, -728, + 24808, 21904, 19008, 16704, 14984, 13512, 12280, 11192, + 10232, 9360, 8576, 7856, 7192, 6576, 6000, 5456, + 4944, 4464, 4008, 3576, 3168, 2776, 2400, 2032, + 1688, 1360, 1040, 728, 432, 136, -432, -136 +}; + +static const int16_t *low_inv_quants[3] = { low_inv_quant6, low_inv_quant5, + low_inv_quant4 };
these tables possible can be divided by 2^C and some >> reduced [..] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates
On Sun, 5 Sep 2010, Michael Niedermayer wrote:
On Fri, Aug 06, 2010 at 09:24:23PM +0300, Martin Storsj? wrote:
Hi,
The G.722 decoder patch that Kenan Gillet took a far way through the review process last year seemed to have stalled, I'm trying to pick it up and get it committed.
On Fri, 10 Apr 2009, Michael Niedermayer wrote:
On Thu, Apr 09, 2009 at 08:14:58AM -0700, Kenan Gillet wrote: [...]
+ * @note For the 56000bps and 48000bps bitrates, the respectively 7 and 6 bits + * codeword might be packed, so unpacking might be needed either + * internally or as a separate parser.
do non packed cases exist anywhere in the wild? do packed cases exist anywhere in the wild?
it seems to me that filling 30% of the bits with 0 is not particlarely likely in a real world environment
According to the spec, the point of the lower bitrate versions is to be able to use 1 or 2 bits per byte as an auxillary data channel. The encoder always outputs the 64 kbps mode, but you can choose to overwrite the lowest two bits if you want to.
The decoder is able to cope with these bits being overwritten with random bits, but you get slightly better results if you tell the decoder that this has happened and so it should disregard those bits.
With that in mind, as far as I can interpret the spec, it's the lowest bits that should be skipped for those modes, but the source for this decoder seems to think otherwise. The implementation in spandsp (that has some common heritage with this code base, iirc, does things this way, too.
this does not awnser my question
do non packed cases exist anywhere in the wild?
Not that I know of. The only place in the wild where I know G.722 is used at all is within RTP, and that uses the full bitrate version.
do packed cases exist anywhere in the wild?
Not that I know of.
this matters because the packed case is usefull due to lower bitrate but its not supported while the non packed case is supported in a way that you say doesnt comply to the spec and i cant see anyone using it except by mistake or "because some spec says so"
Yes, I agree. I changed the non-packed mode to comply with the spec, which to me seems to be the sanest thing to do until someone can point to a proper in the wild example of something else in some container.
[...]
+static const int16_t low_inv_quant5[32] = { + -280, -280, -23352, -17560, -14120, -11664, -9752, -8184, + -6864, -5712, -4696, -3784, -2960, -2208, -1520, -880, + 23352, 17560, 14120, 11664, 9752, 8184, 6864, 5712, + 4696, 3784, 2960, 2208, 1520, 880, 280, -280 +}; +static const int16_t low_inv_quant6[64] = { + -136, -136, -136, -136, -24808, -21904, -19008, -16704, + -14984, -13512, -12280, -11192, -10232, -9360, -8576, -7856, + -7192, -6576, -6000, -5456, -4944, -4464, -4008, -3576, + -3168, -2776, -2400, -2032, -1688, -1360, -1040, -728, + 24808, 21904, 19008, 16704, 14984, 13512, 12280, 11192, + 10232, 9360, 8576, 7856, 7192, 6576, 6000, 5456, + 4944, 4464, 4008, 3576, 3168, 2776, 2400, 2032, + 1688, 1360, 1040, 728, 432, 136, -432, -136 +}; + +static const int16_t *low_inv_quants[3] = { low_inv_quant6, low_inv_quant5, + low_inv_quant4 };
these tables possible can be divided by 2^C and some >> reduced
Yes, divided all inv_quant tables by 8 and adjusted the shifting. New patches attached. // Martin
On Mon, Sep 06, 2010 at 01:07:04AM +0300, Martin Storsj? wrote: [...]
+/** + * adaptive predictor + * + * @param cur_diff the dequantized and scaled delta calculated from the + * current codeword + * @note On x86 using the MULL macro in a loop is slower than not using + * the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_qtzd_reconst; + + const int cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]]; + band->part_reconst_mem[1] = band->part_reconst_mem[0]; + band->part_reconst_mem[0] = cur_part_reconst; + + band->pole_mem[1] = av_clip((sg[0] * av_clip(band->pole_mem[0], -8191, 8191) >> 5) + + (sg[1] << 7) + MULL(band->pole_mem[1], 127, 7), -12288, 12288);
this doesnt need a MULL and i suspect its not the only one [...]
+static av_cold int g722_init(AVCodecContext * avctx) +{ + G722Context *c = (G722Context *) avctx->priv_data;
unneeded cast [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire
On Tue, 7 Sep 2010, Michael Niedermayer wrote:
On Mon, Sep 06, 2010 at 01:07:04AM +0300, Martin Storsj? wrote: [...]
+/** + * adaptive predictor + * + * @param cur_diff the dequantized and scaled delta calculated from the + * current codeword + * @note On x86 using the MULL macro in a loop is slower than not using + * the macro. + */ +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_qtzd_reconst; + + const int cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]]; + band->part_reconst_mem[1] = band->part_reconst_mem[0]; + band->part_reconst_mem[0] = cur_part_reconst; + + band->pole_mem[1] = av_clip((sg[0] * av_clip(band->pole_mem[0], -8191, 8191) >> 5) + + (sg[1] << 7) + MULL(band->pole_mem[1], 127, 7), -12288, 12288);
this doesnt need a MULL and i suspect its not the only one
True. Actually all MULL's could be removed just fine.
[...]
+static av_cold int g722_init(AVCodecContext * avctx) +{ + G722Context *c = (G722Context *) avctx->priv_data;
unneeded cast
Removed. Updated patch attached. // Martin
On date Wednesday 2010-09-08 00:13:07 +0300, Martin Storsj? encoded: [...]
+AVCodec adpcm_g722_decoder = { + .name = "g722",
+ .type = CODEC_TYPE_AUDIO,
AVMEDIA_TYPE_AUDIO [...] Regards. -- FFmpeg = Fiendish Fabulous Meaningless Portentous Elegant Gadget
On Wed, 8 Sep 2010, Stefano Sabatini wrote:
On date Wednesday 2010-09-08 00:13:07 +0300, Martin Storsj? encoded: [...]
+AVCodec adpcm_g722_decoder = { + .name = "g722",
+ .type = CODEC_TYPE_AUDIO,
AVMEDIA_TYPE_AUDIO
Hmm, I thought I changed this already. Thanks for pointing it out. Updated patch attached. // Martin
On Wed, Sep 08, 2010 at 09:18:21AM +0300, Martin Storsj? wrote:
On Wed, 8 Sep 2010, Stefano Sabatini wrote:
On date Wednesday 2010-09-08 00:13:07 +0300, Martin Storsj? encoded: [...]
+AVCodec adpcm_g722_decoder = { + .name = "g722",
+ .type = CODEC_TYPE_AUDIO,
AVMEDIA_TYPE_AUDIO
Hmm, I thought I changed this already. Thanks for pointing it out. Updated patch attached. [...] +static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_qtzd_reconst; + + const int cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]]; + band->part_reconst_mem[1] = band->part_reconst_mem[0]; + band->part_reconst_mem[0] = cur_part_reconst; + + band->pole_mem[1] = av_clip((sg[0] * av_clip(band->pole_mem[0], -8191, 8191) >> 5) + + (sg[1] << 7) + (band->pole_mem[1] * 127 >> 7), -12288, 12288); + + limit = 15360 - band->pole_mem[1]; + band->pole_mem[0] = av_clip(-192 * sg[0] + (band->pole_mem[0] * 255 >> 8), -limit, limit); + + + if (cur_diff) { + for (i = 0; i < 6; i++) + band->zero_mem[i] = ((band->zero_mem[i]*255) >> 8) + + ((band->diff_mem[i]^cur_diff) < 0 ? -128 : 128); + } else
+ for (i = 0; i < 6; i++) + band->zero_mem[i] = (band->zero_mem[i]*255) >> 8;
+ + for (i = 5; i > 0; i--) + band->diff_mem[i] = band->diff_mem[i-1]; + band->diff_mem[0] = av_clip_int16(cur_diff << 1); + + band->s_zero = 0; + for (i = 5; i >= 0; i--) + band->s_zero += (band->zero_mem[i]*band->diff_mem[i]) >> 15; + + + cur_qtzd_reconst = av_clip_int16((band->s_predictor + cur_diff) << 1); + band->s_predictor = av_clip_int16(band->s_zero + + (band->pole_mem[0] * cur_qtzd_reconst >> 15) + + (band->pole_mem[1] * band->prev_qtzd_reconst >> 15)); + band->prev_qtzd_reconst = cur_qtzd_reconst; +}
somehow iam feeling this function should return s_predictor and not void
+
+static int inline linear_scale_factor(const int log_factor, int shift) +{ + const int wd1 = inv_log2_table[(log_factor >> 6) & 31]; + shift -= log_factor >> 11; + return (shift < 0 ? wd1 << -shift : wd1 >> shift) << 2; +}
the shift parameter can be droped and instead a constant added to the log_factor param for each call also the <<2 can be removed, scale_factor be scaled down by 4 thus and a few shifts readjusted in uses of it [...]
+static av_cold int g722_init(AVCodecContext * avctx) +{ + G722Context *c = avctx->priv_data; + + if (avctx->channels != 1) { + av_log(avctx, AV_LOG_ERROR, "Only mono tracks are allowed.\n"); + return AVERROR_INVALIDDATA; + } + avctx->sample_fmt = SAMPLE_FMT_S16; + + switch (avctx->bit_rate) { + case 64000: + case 56000: + case 48000: + c->bits_per_sample = avctx->bit_rate/8000; + break;
theres a problem here bitrate is the bitrate of the bitstream in libavcodec thus a lower bitrate stream + trash bits has 64000 too you cant distinguish them like this
+ default: + if (avctx->strict_std_compliance >= FF_COMPLIANCE_STRICT) { + av_log(avctx, AV_LOG_ERROR, "Unsupported bitrate [%d] only " + "48Kb, 56Kb and 64Kb are supported.\n", + avctx->bit_rate); + return AVERROR_INVALIDDATA; + } + av_log(avctx, AV_LOG_WARNING, "Unsupported bitrate [%d], trying with " + "64Kb.\n", avctx->bit_rate); + avctx->bit_rate = 64000; + c->bits_per_sample = 8; + } +
+ if (avctx->sample_rate != 8000 && avctx->sample_rate != 16000) { + av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate [%d] only 8KHz " + "and 16KHz are supported.\n", + avctx->sample_rate); + return AVERROR_INVALIDDATA; + }
this looks lame, the code surely works too with 22khz or 44khz or anything else, its just a stream of integers that are being compressed or decompressed the samplerate is not essential [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you really think that XML is the answer, then you definitly missunderstood the question -- Attila Kinali
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
On Wed, Sep 08, 2010 at 09:18:21AM +0300, Martin Storsj? wrote:
+static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_qtzd_reconst; + + const int cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]]; + band->part_reconst_mem[1] = band->part_reconst_mem[0]; + band->part_reconst_mem[0] = cur_part_reconst; + + band->pole_mem[1] = av_clip((sg[0] * av_clip(band->pole_mem[0], -8191, 8191) >> 5) + + (sg[1] << 7) + (band->pole_mem[1] * 127 >> 7), -12288, 12288); + + limit = 15360 - band->pole_mem[1]; + band->pole_mem[0] = av_clip(-192 * sg[0] + (band->pole_mem[0] * 255 >> 8), -limit, limit); + + + if (cur_diff) { + for (i = 0; i < 6; i++) + band->zero_mem[i] = ((band->zero_mem[i]*255) >> 8) + + ((band->diff_mem[i]^cur_diff) < 0 ? -128 : 128); + } else
+ for (i = 0; i < 6; i++) + band->zero_mem[i] = (band->zero_mem[i]*255) >> 8;
+ + for (i = 5; i > 0; i--) + band->diff_mem[i] = band->diff_mem[i-1]; + band->diff_mem[0] = av_clip_int16(cur_diff << 1); + + band->s_zero = 0; + for (i = 5; i >= 0; i--) + band->s_zero += (band->zero_mem[i]*band->diff_mem[i]) >> 15; + + + cur_qtzd_reconst = av_clip_int16((band->s_predictor + cur_diff) << 1); + band->s_predictor = av_clip_int16(band->s_zero + + (band->pole_mem[0] * cur_qtzd_reconst >> 15) + + (band->pole_mem[1] * band->prev_qtzd_reconst >> 15)); + band->prev_qtzd_reconst = cur_qtzd_reconst; +}
somehow iam feeling this function should return s_predictor and not void
Yes, that would perhaps be logical, however, in this codec, the updated predictor value isn't used until decoding the next sample, due to the scalable bitrate design (where different inv quant tables are used for decoding the actual samples, but the one same lower precision one always is used for updating the predictor internally, regardless which way it is decoded). So making this return the predictor wouldn't really help anything in this implementation.
+static int inline linear_scale_factor(const int log_factor, int shift) +{ + const int wd1 = inv_log2_table[(log_factor >> 6) & 31]; + shift -= log_factor >> 11; + return (shift < 0 ? wd1 << -shift : wd1 >> shift) << 2; +}
the shift parameter can be droped and instead a constant added to the log_factor param for each call
Yes, dropped this parameter.
also the <<2 can be removed, scale_factor be scaled down by 4 thus and a few shifts readjusted in uses of it
Hmm, I'm not sure I'm following you here. Do you mean that I should premultiply inv_log2_table by 4 and skip the final right shift? Or just add/subtract 2 from the 'shift' variable? I tried that (the former, but the latter would have the same effect), but it doesn't give bitexact output, since the codec relies on truncating the values when doing the right shift.
+static av_cold int g722_init(AVCodecContext * avctx) +{ + G722Context *c = avctx->priv_data; + + if (avctx->channels != 1) { + av_log(avctx, AV_LOG_ERROR, "Only mono tracks are allowed.\n"); + return AVERROR_INVALIDDATA; + } + avctx->sample_fmt = SAMPLE_FMT_S16; + + switch (avctx->bit_rate) { + case 64000: + case 56000: + case 48000: + c->bits_per_sample = avctx->bit_rate/8000; + break;
theres a problem here bitrate is the bitrate of the bitstream in libavcodec thus a lower bitrate stream + trash bits has 64000 too you cant distinguish them like this
Yes, this is a problem. Do you have any other suggestion for how to signal the lower bitrate variants? Would AVCodecContext->bits_per_coded_sample perhaps be suitable for this?
+ if (avctx->sample_rate != 8000 && avctx->sample_rate != 16000) { + av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate [%d] only 8KHz " + "and 16KHz are supported.\n", + avctx->sample_rate); + return AVERROR_INVALIDDATA; + }
this looks lame, the code surely works too with 22khz or 44khz or anything else, its just a stream of integers that are being compressed or decompressed the samplerate is not essential
Yes, that's true. The codec itself is only specified for 16 kHz, but there's nothing stopping the user from using it on any data, as you said. This restriction was earlier used to be able to decode only the lower subband if 8 kHz was chosen, but I'm removing that function for now - if it is desired, I guess it should be signalled in some other way. Updated patch attached. // Martin
On Wed, Sep 08, 2010 at 04:38:36PM +0300, Martin Storsj? wrote:
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
On Wed, Sep 08, 2010 at 09:18:21AM +0300, Martin Storsj? wrote:
+static void do_adaptive_prediction(struct G722Band *band, const int cur_diff) +{ + int sg[2], limit, i, cur_qtzd_reconst; + + const int cur_part_reconst = band->s_zero + cur_diff < 0; + + sg[0] = sign_lookup[cur_part_reconst != band->part_reconst_mem[0]]; + sg[1] = sign_lookup[cur_part_reconst == band->part_reconst_mem[1]]; + band->part_reconst_mem[1] = band->part_reconst_mem[0]; + band->part_reconst_mem[0] = cur_part_reconst; + + band->pole_mem[1] = av_clip((sg[0] * av_clip(band->pole_mem[0], -8191, 8191) >> 5) + + (sg[1] << 7) + (band->pole_mem[1] * 127 >> 7), -12288, 12288); + + limit = 15360 - band->pole_mem[1]; + band->pole_mem[0] = av_clip(-192 * sg[0] + (band->pole_mem[0] * 255 >> 8), -limit, limit); + + + if (cur_diff) { + for (i = 0; i < 6; i++) + band->zero_mem[i] = ((band->zero_mem[i]*255) >> 8) + + ((band->diff_mem[i]^cur_diff) < 0 ? -128 : 128); + } else
+ for (i = 0; i < 6; i++) + band->zero_mem[i] = (band->zero_mem[i]*255) >> 8;
+ + for (i = 5; i > 0; i--) + band->diff_mem[i] = band->diff_mem[i-1]; + band->diff_mem[0] = av_clip_int16(cur_diff << 1); + + band->s_zero = 0; + for (i = 5; i >= 0; i--) + band->s_zero += (band->zero_mem[i]*band->diff_mem[i]) >> 15; + + + cur_qtzd_reconst = av_clip_int16((band->s_predictor + cur_diff) << 1); + band->s_predictor = av_clip_int16(band->s_zero + + (band->pole_mem[0] * cur_qtzd_reconst >> 15) + + (band->pole_mem[1] * band->prev_qtzd_reconst >> 15)); + band->prev_qtzd_reconst = cur_qtzd_reconst; +}
somehow iam feeling this function should return s_predictor and not void
Yes, that would perhaps be logical, however, in this codec, the updated predictor value isn't used until decoding the next sample, due to the scalable bitrate design (where different inv quant tables are used for decoding the actual samples, but the one same lower precision one always is used for updating the predictor internally, regardless which way it is decoded). So making this return the predictor wouldn't really help anything in this implementation.
+static int inline linear_scale_factor(const int log_factor, int shift) +{ + const int wd1 = inv_log2_table[(log_factor >> 6) & 31]; + shift -= log_factor >> 11; + return (shift < 0 ? wd1 << -shift : wd1 >> shift) << 2; +}
the shift parameter can be droped and instead a constant added to the log_factor param for each call
Yes, dropped this parameter.
also the <<2 can be removed, scale_factor be scaled down by 4 thus and a few shifts readjusted in uses of it
Hmm, I'm not sure I'm following you here. Do you mean that I should premultiply inv_log2_table by 4 and skip the final right shift? Or just add/subtract 2 from the 'shift' variable? I tried that (the former, but the latter would have the same effect), but it doesn't give bitexact output, since the codec relies on truncating the values when doing the right shift.
ive meant making scale_factor= scale_factor/4 and simplifying all code that reads or writes it. This seems possible, am i missing something?
+static av_cold int g722_init(AVCodecContext * avctx) +{ + G722Context *c = avctx->priv_data; + + if (avctx->channels != 1) { + av_log(avctx, AV_LOG_ERROR, "Only mono tracks are allowed.\n"); + return AVERROR_INVALIDDATA; + } + avctx->sample_fmt = SAMPLE_FMT_S16; + + switch (avctx->bit_rate) { + case 64000: + case 56000: + case 48000: + c->bits_per_sample = avctx->bit_rate/8000; + break;
theres a problem here bitrate is the bitrate of the bitstream in libavcodec thus a lower bitrate stream + trash bits has 64000 too you cant distinguish them like this
Yes, this is a problem. Do you have any other suggestion for how to signal the lower bitrate variants? Would AVCodecContext->bits_per_coded_sample perhaps be suitable for this?
maybe [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The misfortune of the wise is better than the prosperity of the fool. -- Epicurus
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
On Wed, Sep 08, 2010 at 04:38:36PM +0300, Martin Storsj? wrote:
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
also the <<2 can be removed, scale_factor be scaled down by 4 thus and a few shifts readjusted in uses of it
Hmm, I'm not sure I'm following you here. Do you mean that I should premultiply inv_log2_table by 4 and skip the final right shift? Or just add/subtract 2 from the 'shift' variable? I tried that (the former, but the latter would have the same effect), but it doesn't give bitexact output, since the codec relies on truncating the values when doing the right shift.
ive meant making scale_factor= scale_factor/4 and simplifying all code that reads or writes it. This seems possible, am i missing something?
Ah, yes, now I see. Yes, that's possible - changed it that way.
+static av_cold int g722_init(AVCodecContext * avctx) +{ + G722Context *c = avctx->priv_data; + + if (avctx->channels != 1) { + av_log(avctx, AV_LOG_ERROR, "Only mono tracks are allowed.\n"); + return AVERROR_INVALIDDATA; + } + avctx->sample_fmt = SAMPLE_FMT_S16; + + switch (avctx->bit_rate) { + case 64000: + case 56000: + case 48000: + c->bits_per_sample = avctx->bit_rate/8000; + break;
theres a problem here bitrate is the bitrate of the bitstream in libavcodec thus a lower bitrate stream + trash bits has 64000 too you cant distinguish them like this
Yes, this is a problem. Do you have any other suggestion for how to signal the lower bitrate variants? Would AVCodecContext->bits_per_coded_sample perhaps be suitable for this?
maybe
Ok, changed to use that instead, that sure does look much better. Updated patch attached. // Martin
On Wed, Sep 08, 2010 at 06:06:42PM +0300, Martin Storsj? wrote: [...]
+static int g722_decode_frame(AVCodecContext *avctx, void *data, + int *data_size, AVPacket *avpkt) +{ + G722Context *c = avctx->priv_data; + int16_t *out_buf = data; + int j, out_len = 0; + const int skip = 8 - avctx->bits_per_coded_sample; + const int16_t *quantizer_table = low_inv_quants[skip]; + GetBitContext gb; + + init_get_bits(&gb, avpkt->data, avpkt->size * 8); + + for (j = 0; j < avpkt->size; j++) { + int ilow, ihigh, rlow; + int dhigh, rhigh; + int xout1, xout2; + + ihigh = get_bits(&gb, 2); + ilow = get_bits(&gb, 6 - skip); + skip_bits(&gb, skip); + + rlow = av_clip((c->band[0].scale_factor * quantizer_table[ilow] >> 10) + + c->band[0].s_predictor, -16384, 16383); + update_low_predictor(&c->band[0], ilow >> (2 - skip)); + + dhigh = c->band[1].scale_factor * high_inv_quant[ihigh] >> 10; + rhigh = av_clip(dhigh + c->band[1].s_predictor, -16384, 16383); + update_high_predictor(&c->band[1], dhigh, ihigh); + + c->prev_samples[c->prev_samples_pos++] = rlow + rhigh; + c->prev_samples[c->prev_samples_pos++] = rlow - rhigh; + apply_qmf(c->prev_samples + c->prev_samples_pos - 24, + &xout1, &xout2); + out_buf[out_len++] = av_clip_int16(xout1 >> 12); + out_buf[out_len++] = av_clip_int16(xout2 >> 12); + if (c->prev_samples_pos >= PREV_SAMPLES_BUF_SIZE) { + memmove(c->prev_samples, + c->prev_samples + c->prev_samples_pos - 22, + 22 * sizeof(c->prev_samples[0])); + c->prev_samples_pos = 22; + }
lowres could be checked for this except that iam fine with the patch [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many things microsoft did are stupid, but not doing something just because microsoft did it is even more stupid. If everything ms did were stupid they would be bankrupt already.
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
On Wed, Sep 08, 2010 at 06:06:42PM +0300, Martin Storsj? wrote: [...]
+static int g722_decode_frame(AVCodecContext *avctx, void *data, + int *data_size, AVPacket *avpkt) +{ + G722Context *c = avctx->priv_data; + int16_t *out_buf = data; + int j, out_len = 0; + const int skip = 8 - avctx->bits_per_coded_sample; + const int16_t *quantizer_table = low_inv_quants[skip]; + GetBitContext gb; + + init_get_bits(&gb, avpkt->data, avpkt->size * 8); + + for (j = 0; j < avpkt->size; j++) { + int ilow, ihigh, rlow; + int dhigh, rhigh; + int xout1, xout2; + + ihigh = get_bits(&gb, 2); + ilow = get_bits(&gb, 6 - skip); + skip_bits(&gb, skip); + + rlow = av_clip((c->band[0].scale_factor * quantizer_table[ilow] >> 10) + + c->band[0].s_predictor, -16384, 16383); + update_low_predictor(&c->band[0], ilow >> (2 - skip)); + + dhigh = c->band[1].scale_factor * high_inv_quant[ihigh] >> 10; + rhigh = av_clip(dhigh + c->band[1].s_predictor, -16384, 16383); + update_high_predictor(&c->band[1], dhigh, ihigh); + + c->prev_samples[c->prev_samples_pos++] = rlow + rhigh; + c->prev_samples[c->prev_samples_pos++] = rlow - rhigh; + apply_qmf(c->prev_samples + c->prev_samples_pos - 24, + &xout1, &xout2); + out_buf[out_len++] = av_clip_int16(xout1 >> 12); + out_buf[out_len++] = av_clip_int16(xout2 >> 12); + if (c->prev_samples_pos >= PREV_SAMPLES_BUF_SIZE) { + memmove(c->prev_samples, + c->prev_samples + c->prev_samples_pos - 22, + 22 * sizeof(c->prev_samples[0])); + c->prev_samples_pos = 22; + }
lowres could be checked for this
Ah, so if avctx->lowres == 1, we'd just output the lower subband? Good idea, I'll try to implement that tomorrow.
except that iam fine with the patch
Good, I'll resend once more when I've implemented the lowres option. // Martin
On Thu, 9 Sep 2010, Martin Storsj? wrote:
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
On Wed, Sep 08, 2010 at 06:06:42PM +0300, Martin Storsj? wrote:
lowres could be checked for this
Ah, so if avctx->lowres == 1, we'd just output the lower subband? Good idea, I'll try to implement that tomorrow.
Implemented it, and it works more or less. I had to add an A flag to the lowres option in lavc/options.c. Also, if lowres is enabled, I set the sample rate to the half in g722_init, but it doesn't propagate automatically to the output when transcoding with ffmpeg, since it probably copies the original sample rate before opening the decoder. // Martin
Martin Storsj? <martin <at> martin.st> writes:
Implemented it, and it works more or less. I had to add an A flag to the lowres option in lavc/options.c. Also, if lowres is enabled, I set the sample rate to the half in g722_init, but it doesn't propagate automatically to the output when transcoding with ffmpeg, since it probably copies the original sample rate before opening the decoder.
I suspect you have to right shift the sample_rate in avcodec_open by lowres (and remove AVMEDIA_TYPE_VIDEO from the lowres sanity check and change your decoder definition accordingly). Carl Eugen
On Thu, 9 Sep 2010, Carl Eugen Hoyos wrote:
Martin Storsj? <martin <at> martin.st> writes:
Implemented it, and it works more or less. I had to add an A flag to the lowres option in lavc/options.c. Also, if lowres is enabled, I set the sample rate to the half in g722_init, but it doesn't propagate automatically to the output when transcoding with ffmpeg, since it probably copies the original sample rate before opening the decoder.
I suspect you have to right shift the sample_rate in avcodec_open by lowres (and remove AVMEDIA_TYPE_VIDEO from the lowres sanity check and change your decoder definition accordingly).
I don't think that would help. The original sample rate is copied to the output encoder before the input decoder is opened, and thus it doesn't matter if the shifting happens in avcodec_open or the codec's init function, which is called within avcodec_open. // Martin
On Thu, Sep 09, 2010 at 11:04:39AM +0300, Martin Storsj? wrote:
On Thu, 9 Sep 2010, Carl Eugen Hoyos wrote:
Martin Storsj? <martin <at> martin.st> writes:
Implemented it, and it works more or less. I had to add an A flag to the lowres option in lavc/options.c.
ok and i think you should look at how width/height is handled [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct awnser.
On Thu, 9 Sep 2010, Michael Niedermayer wrote:
On Thu, Sep 09, 2010 at 11:04:39AM +0300, Martin Storsj? wrote:
On Thu, 9 Sep 2010, Carl Eugen Hoyos wrote:
Martin Storsj? <martin <at> martin.st> writes:
Implemented it, and it works more or less. I had to add an A flag to the lowres option in lavc/options.c.
ok and i think you should look at how width/height is handled
Ah, yes, now I found it - ffmpeg.c shifts the width/height (used for the output) if lowres is set, before opening the decoder - trivial to do the same for audio. Updated patch set attached, should be just fine now, afaik. // Martin
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
lowres could be checked for this except that iam fine with the patch
Applied the decoder and demuxer/muxer. I noticed that a separate AVOption (which I named -audio_lowres) is needed for setting lowres for audio streams, otherwise it's impossible to specify which stream the -lowres parameter applies to, as in the previous patch. Updated patch for the lowres option attached. // Martin
On Thu, Sep 09, 2010 at 10:36:41PM +0300, Martin Storsj? wrote:
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
lowres could be checked for this except that iam fine with the patch
Applied the decoder and demuxer/muxer. I noticed that a separate AVOption (which I named -audio_lowres) is needed for setting lowres for audio
-alowres should work without this "a..." should be routed to the audio context [...]
@@ -3241,6 +3241,8 @@ static void opt_input_file(const char *filename) input_codecs[nb_icodecs++] = avcodec_find_decoder_by_name(audio_codec_name); if(audio_disable) st->discard= AVDISCARD_ALL; + if (dec->lowres) + audio_sample_rate >>= dec->lowres; break; case AVMEDIA_TYPE_VIDEO: set_context_opts(dec, avcodec_opts[AVMEDIA_TYPE_VIDEO], AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM);
with a comment that says av_find_stream_info() can add streams and we have no chance to set lowres up for them early enough ... this hunk should be ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
On Thu, 9 Sep 2010, Michael Niedermayer wrote:
On Thu, Sep 09, 2010 at 10:36:41PM +0300, Martin Storsj? wrote:
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
lowres could be checked for this except that iam fine with the patch
Applied the decoder and demuxer/muxer. I noticed that a separate AVOption (which I named -audio_lowres) is needed for setting lowres for audio
-alowres should work without this "a..." should be routed to the audio context
Oh, nifty, didn't know about that!
@@ -3241,6 +3241,8 @@ static void opt_input_file(const char *filename) input_codecs[nb_icodecs++] = avcodec_find_decoder_by_name(audio_codec_name); if(audio_disable) st->discard= AVDISCARD_ALL; + if (dec->lowres) + audio_sample_rate >>= dec->lowres; break; case AVMEDIA_TYPE_VIDEO: set_context_opts(dec, avcodec_opts[AVMEDIA_TYPE_VIDEO], AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM);
with a comment that says av_find_stream_info() can add streams and we have no chance to set lowres up for them early enough ... this hunk should be ok
Applied this hunk. The rest of it attached again. // Martin
On Fri, Sep 10, 2010 at 12:00:19AM +0300, Martin Storsj? wrote:
On Thu, 9 Sep 2010, Michael Niedermayer wrote:
On Thu, Sep 09, 2010 at 10:36:41PM +0300, Martin Storsj? wrote:
On Wed, 8 Sep 2010, Michael Niedermayer wrote:
lowres could be checked for this except that iam fine with the patch
Applied the decoder and demuxer/muxer. I noticed that a separate AVOption (which I named -audio_lowres) is needed for setting lowres for audio
-alowres should work without this "a..." should be routed to the audio context
Oh, nifty, didn't know about that!
@@ -3241,6 +3241,8 @@ static void opt_input_file(const char *filename) input_codecs[nb_icodecs++] = avcodec_find_decoder_by_name(audio_codec_name); if(audio_disable) st->discard= AVDISCARD_ALL; + if (dec->lowres) + audio_sample_rate >>= dec->lowres; break; case AVMEDIA_TYPE_VIDEO: set_context_opts(dec, avcodec_opts[AVMEDIA_TYPE_VIDEO], AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM);
with a comment that says av_find_stream_info() can add streams and we have no chance to set lowres up for them early enough ... this hunk should be ok
Applied this hunk.
The rest of it attached again.
// Martin options.c | 2 +- utils.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) 782f446994fb505825aa1f67e3c7c5a35120b610 0001-Allow-the-lowres-option-to-affect-audio-codecs-too.patch From 2fc7da9c196fc11ca56dc92acca4390481cda5ea Mon Sep 17 00:00:00 2001 From: Martin Storsjo <martin at martin.st> Date: Thu, 9 Sep 2010 09:08:34 +0300 Subject: [PATCH] Allow the lowres option to affect audio codecs, too
--- libavcodec/options.c | 2 +- libavcodec/utils.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
looks good if it passes tests [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle
On Fri, 10 Sep 2010, Michael Niedermayer wrote:
On Fri, Sep 10, 2010 at 12:00:19AM +0300, Martin Storsj? wrote:
The rest of it attached again.
// Martin options.c | 2 +- utils.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) 782f446994fb505825aa1f67e3c7c5a35120b610 0001-Allow-the-lowres-option-to-affect-audio-codecs-too.patch From 2fc7da9c196fc11ca56dc92acca4390481cda5ea Mon Sep 17 00:00:00 2001 From: Martin Storsjo <martin at martin.st> Date: Thu, 9 Sep 2010 09:08:34 +0300 Subject: [PATCH] Allow the lowres option to affect audio codecs, too
--- libavcodec/options.c | 2 +- libavcodec/utils.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
looks good if it passes tests
Yes, all tests passed locally, applied. Thanks for the reviewing! // Martin
participants (7)
-
cehoyos＠ag.or.at -
diego＠biurrun.de -
kenan.gillet＠gmail.com -
martin＠martin.st -
michaelni＠gmx.at -
rsbultje＠gmail.com -
stefano.sabatini-lala＠poste.it