[Ffmpeg-devel] [PATCH] GSM-MS decoder and encoder

[Michael Niedermayer]

Hi

On Sat, Feb 17, 2007 at 03:42:52PM +0100, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> > Hi
> > 
> > On Wed, Feb 14, 2007 at 03:29:23PM +0100, Michel Bardiaux wrote:
> > [...]
> > 
> >> // gsm.h miss some essential constants
> >> #define GSM_BLOCK_SIZE 33
> >>+#define GSM_MS_BLOCK_SIZE 65
> >> #define GSM_FRAME_SIZE 160
> >> 
> >> static int libgsm_init(AVCodecContext *avctx) {
> >>-    if (avctx->channels > 1 || avctx->sample_rate != 8000)
> >>+    if (avctx->channels > 1 || avctx->sample_rate != 8000 || avctx->bit_rate != 13000)
> > 
> > 
> > is 13000 exactly correct isnt it 13200 for CODEC_ID_GSM?
> 
> Yes and no. The last 4 bits of each frame are not actually used, they
> come on stage only because we (both libgsm and lavc) use a byte-oriented
> API, and files made of bytes. They should be considered as container
> overhead in TOAST files, hence not part of the codec bitrate.

but with this argumentation we would have to subtract the padding bits from
mpeg from the bitrate too and thats something we dont also it would cause
problems for containers which expect the bitrate well to be the bitrate of
what they get, not to be slightly less due to some padding bits they dont
know about ...


> 
> (A lot of people would have been spared a lot of sweat if ETSI had
> simply required these 4 bits as mandatory zero padding, and had
> specified the order of bits in bytes, which proves again that common
> sense is not an attribute of standardization bodies...)

fully agree


> 
> > [...]
> > 
> >>@@ -356,6 +357,10 @@
> >>         put_le16(pb, 16); /* fwHeadFlags */
> >>         put_le32(pb, 0);  /* dwPTSLow */
> >>         put_le32(pb, 0);  /* dwPTSHigh */
> >>+    } else if (enc->codec_id == CODEC_ID_GSM_MS) {
> >>+        put_le16(pb, 2); /* wav_extra_size */
> >>+        hdrsize += 2;
> >>+        put_le16(pb, 320); /* wSamplesPerBlock */
> > 
> > 
> > isnt this simply avctx->frame_size ? i mean hardcoding 320 looks a little ugly
> > unless its really needed ...
> 
> Right. Changed that. But then you must be very unhappy about the next
> lines in that source:
> 
>     } else if (enc->codec_id == CODEC_ID_ADPCM_IMA_WAV) {
>         put_le16(pb, 2); /* wav_extra_size */
>         hdrsize += 2;
>         put_le16(pb, ((enc->block_align - 4 * enc->channels) / (4 *
> enc->channels)) * 8 + 1); /* wSamplesPerBlock */
> 
> which duplicate what is in adpcm_encode_init. Should I change them in
> another patch (there doesnt seem to be an official maintainer for riff.c)

simplifications are always welcome ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070217/a382f22a/attachment.pgp>