[2/2] g722: Change bits per sample to 4

Message ID 1322936044-58230-2-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö Dec. 3, 2011, 6:14 p.m.
From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

Earlier, bits per sample was defined as 8, since
bits_per_coded_sample was used to indicate whether to ignore
the lower bits of the codeword, having values 6, 7 or 8.

g722 encodes 2 samples into one byte codeword, therefore the
bits per sample is 4. By changing this, the generated timestamps
for streams encoded with g722 become correct.

This makes timestamp generation for g722 data correct (both when
encoding and when demuxing from raw g722 files).
---
 libavcodec/g722enc.c |    4 ++--
 libavcodec/utils.c   |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Justin Ruggles Dec. 3, 2011, 7:25 p.m. | #1
On 12/03/2011 01:14 PM, Martin Storsjö wrote:

> From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> 
> Earlier, bits per sample was defined as 8, since
> bits_per_coded_sample was used to indicate whether to ignore
> the lower bits of the codeword, having values 6, 7 or 8.
> 
> g722 encodes 2 samples into one byte codeword, therefore the
> bits per sample is 4. By changing this, the generated timestamps
> for streams encoded with g722 become correct.
> 
> This makes timestamp generation for g722 data correct (both when
> encoding and when demuxing from raw g722 files).
> ---
>  libavcodec/g722enc.c |    4 ++--
>  libavcodec/utils.c   |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)


I assume we don't have a g722-in-wav encoding test in FATE?  Because
this changes the WAVE header bits-per-sample field from 8 to 4 when
muxing.  If we don't have one, maybe we should add it. :)

-Justin
Martin Storsjö Dec. 3, 2011, 9:33 p.m. | #2
On Sat, 3 Dec 2011, Justin Ruggles wrote:

> On 12/03/2011 01:14 PM, Martin Storsjö wrote:
>
>> From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>
>> Earlier, bits per sample was defined as 8, since
>> bits_per_coded_sample was used to indicate whether to ignore
>> the lower bits of the codeword, having values 6, 7 or 8.
>>
>> g722 encodes 2 samples into one byte codeword, therefore the
>> bits per sample is 4. By changing this, the generated timestamps
>> for streams encoded with g722 become correct.
>>
>> This makes timestamp generation for g722 data correct (both when
>> encoding and when demuxing from raw g722 files).
>> ---
>>  libavcodec/g722enc.c |    4 ++--
>>  libavcodec/utils.c   |    2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>
>
> I assume we don't have a g722-in-wav encoding test in FATE?  Because
> this changes the WAVE header bits-per-sample field from 8 to 4 when
> muxing.  If we don't have one, maybe we should add it. :)

We most probably don't have such a test :-)

Would it be something like this?

fate-wav-g722enc: CMD = md5 -ar 16000 -ac 1 -f s16le -i 
$(TARGET_PATH)/tests/data/asynth-16000-1.sw -acodec g722 -ac 1 -f wav

That is, just like the other g722enc test, encoding from a synthetic test 
file and just testing the hash of the full muxed file?

// Martin
Justin Ruggles Dec. 3, 2011, 9:43 p.m. | #3
On 12/03/2011 04:33 PM, Martin Storsjö wrote:

> On Sat, 3 Dec 2011, Justin Ruggles wrote:
> 
>> On 12/03/2011 01:14 PM, Martin Storsjö wrote:
>>
>>> From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>>
>>> Earlier, bits per sample was defined as 8, since
>>> bits_per_coded_sample was used to indicate whether to ignore
>>> the lower bits of the codeword, having values 6, 7 or 8.
>>>
>>> g722 encodes 2 samples into one byte codeword, therefore the
>>> bits per sample is 4. By changing this, the generated timestamps
>>> for streams encoded with g722 become correct.
>>>
>>> This makes timestamp generation for g722 data correct (both when
>>> encoding and when demuxing from raw g722 files).
>>> ---
>>>  libavcodec/g722enc.c |    4 ++--
>>>  libavcodec/utils.c   |    2 +-
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>>
>> I assume we don't have a g722-in-wav encoding test in FATE?  Because
>> this changes the WAVE header bits-per-sample field from 8 to 4 when
>> muxing.  If we don't have one, maybe we should add it. :)
> 
> We most probably don't have such a test :-)
> 
> Would it be something like this?
> 
> fate-wav-g722enc: CMD = md5 -ar 16000 -ac 1 -f s16le -i 
> $(TARGET_PATH)/tests/data/asynth-16000-1.sw -acodec g722 -ac 1 -f wav
> 
> That is, just like the other g722enc test, encoding from a synthetic test 
> file and just testing the hash of the full muxed file?


both the encoder and decoder are fixed-point, so you can add an acodec
test in codec-regression.sh. like so:

if [ -n "$do_g722" ] ; then
do_audio_encoding g722.wav "-ac 1 -ar 16000 -c:a g726"
do_audio_decoding
fi

-Justin
Justin Ruggles Dec. 3, 2011, 9:45 p.m. | #4
On 12/03/2011 04:43 PM, Justin Ruggles wrote:

> On 12/03/2011 04:33 PM, Martin Storsjö wrote:
> 
>> On Sat, 3 Dec 2011, Justin Ruggles wrote:
>>
>>> On 12/03/2011 01:14 PM, Martin Storsjö wrote:
>>>
>>>> From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>>>
>>>> Earlier, bits per sample was defined as 8, since
>>>> bits_per_coded_sample was used to indicate whether to ignore
>>>> the lower bits of the codeword, having values 6, 7 or 8.
>>>>
>>>> g722 encodes 2 samples into one byte codeword, therefore the
>>>> bits per sample is 4. By changing this, the generated timestamps
>>>> for streams encoded with g722 become correct.
>>>>
>>>> This makes timestamp generation for g722 data correct (both when
>>>> encoding and when demuxing from raw g722 files).
>>>> ---
>>>>  libavcodec/g722enc.c |    4 ++--
>>>>  libavcodec/utils.c   |    2 +-
>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>>
>>> I assume we don't have a g722-in-wav encoding test in FATE?  Because
>>> this changes the WAVE header bits-per-sample field from 8 to 4 when
>>> muxing.  If we don't have one, maybe we should add it. :)
>>
>> We most probably don't have such a test :-)
>>
>> Would it be something like this?
>>
>> fate-wav-g722enc: CMD = md5 -ar 16000 -ac 1 -f s16le -i 
>> $(TARGET_PATH)/tests/data/asynth-16000-1.sw -acodec g722 -ac 1 -f wav
>>
>> That is, just like the other g722enc test, encoding from a synthetic test 
>> file and just testing the hash of the full muxed file?
> 
> 
> both the encoder and decoder are fixed-point, so you can add an acodec
> test in codec-regression.sh. like so:
> 
> if [ -n "$do_g722" ] ; then
> do_audio_encoding g722.wav "-ac 1 -ar 16000 -c:a g726"

                                                   ^^^^
oops. copy/paste error. obviously should be g722.

> do_audio_decoding
> fi


-Justin
Martin Storsjö Dec. 3, 2011, 11:04 p.m. | #5
On Sat, 3 Dec 2011, Justin Ruggles wrote:

> On 12/03/2011 04:33 PM, Martin Storsjö wrote:
>
>> On Sat, 3 Dec 2011, Justin Ruggles wrote:
>>
>>> On 12/03/2011 01:14 PM, Martin Storsjö wrote:
>>>
>>>> From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>>>
>>>> Earlier, bits per sample was defined as 8, since
>>>> bits_per_coded_sample was used to indicate whether to ignore
>>>> the lower bits of the codeword, having values 6, 7 or 8.
>>>>
>>>> g722 encodes 2 samples into one byte codeword, therefore the
>>>> bits per sample is 4. By changing this, the generated timestamps
>>>> for streams encoded with g722 become correct.
>>>>
>>>> This makes timestamp generation for g722 data correct (both when
>>>> encoding and when demuxing from raw g722 files).
>>>> ---
>>>>  libavcodec/g722enc.c |    4 ++--
>>>>  libavcodec/utils.c   |    2 +-
>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>>
>>> I assume we don't have a g722-in-wav encoding test in FATE?  Because
>>> this changes the WAVE header bits-per-sample field from 8 to 4 when
>>> muxing.  If we don't have one, maybe we should add it. :)
>>
>> We most probably don't have such a test :-)
>>
>> Would it be something like this?
>>
>> fate-wav-g722enc: CMD = md5 -ar 16000 -ac 1 -f s16le -i
>> $(TARGET_PATH)/tests/data/asynth-16000-1.sw -acodec g722 -ac 1 -f wav
>>
>> That is, just like the other g722enc test, encoding from a synthetic test
>> file and just testing the hash of the full muxed file?
>
>
> both the encoder and decoder are fixed-point, so you can add an acodec
> test in codec-regression.sh. like so:
>
> if [ -n "$do_g722" ] ; then
> do_audio_encoding g722.wav "-ac 1 -ar 16000 -c:a g726"
> do_audio_decoding
> fi

Ok, I'll send such a patch then.

Is this one ok then?

// Martin

Patch

diff --git a/libavcodec/g722enc.c b/libavcodec/g722enc.c
index bdc30d5..f8db49a 100644
--- a/libavcodec/g722enc.c
+++ b/libavcodec/g722enc.c
@@ -139,7 +139,7 @@  static int g722_encode_trellis(AVCodecContext *avctx,
         nodes[i][0]->state = c->band[i];
     }
 
-    for (i = 0; i < buf_size >> 1; i++) {
+    for (i = 0; i < buf_size; i++) {
         int xlow, xhigh;
         struct TrellisNode *next[2];
         int heap_pos[2] = {0, 0};
@@ -285,7 +285,7 @@  static int g722_encode_frame(AVCodecContext *avctx,
     if (avctx->trellis)
         return g722_encode_trellis(avctx, dst, buf_size, data);
 
-    for (i = 0; i < buf_size >> 1; i++) {
+    for (i = 0; i < buf_size; i++) {
         int xlow, xhigh, ihigh, ilow;
         filter_samples(c, &samples[2*i], &xlow, &xhigh);
         ihigh = encode_high(&c->band[1], xhigh);
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index c844399..04909cf 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -1342,8 +1342,8 @@  int av_get_bits_per_sample(enum CodecID codec_id){
     case CODEC_ID_ADPCM_SWF:
     case CODEC_ID_ADPCM_MS:
     case CODEC_ID_ADPCM_YAMAHA:
-        return 4;
     case CODEC_ID_ADPCM_G722:
+        return 4;
     case CODEC_ID_PCM_ALAW:
     case CODEC_ID_PCM_MULAW:
     case CODEC_ID_PCM_S8: