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

[Michael Niedermayer]

On Tue, Apr 29, 2008 at 11:10:57AM +0200, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> > On Mon, Apr 28, 2008 at 06:56:32PM +0200, Michael Niedermayer wrote:
> >> On Mon, Apr 28, 2008 at 05:16:21PM +0200, Michel Bardiaux wrote:
> >>> Michael Niedermayer wrote:
> >> [...]
> >> 
> >>>>> its the latter that should fix things if the file is
> >>>>> obviously corrupted (which this one is!).
> >>>> I see nothing corrupt on the file
> >>>>> That is, at the end of get_wav_header (proper patch when
> >>>>> there is agreement of the ways and means). Theoretically
> >>>>> there is no maintainer for riff.c but since Michael is
> >>>>> maintainer for the avi muxdemux I guess he is for riff.c too
> >>>>> (should I correct MAINTAINERS?)
> >>>> Yes i mainain riff.c, feel free to add me to the list if you
> >>>> like ...
> >>>>> And whether the demuxer or the codec changes the sample rate,
> >>>>> a warning should be issued. OK?
> >>>> Print as many warnings as you like :) but please dont reject
> >>>> streams at random,
> >>> It is *NOT* at random. The spec is very clear: the sample rate
> >>> for GSM is 8000. Any other value in the RIFF headers is simply
> >>> wrong and hints that the encoder has screwed up.
> >>> 
> >>>> patch below fixes this file and i suspect others as well, i
> >>>> will apply it in 24h unless you object.
> >>> The change was submitted for review and you didn't object... But
> >>> if you insist on a quick-and-dirty fix for all those malformed
> >>> files, patch attached for your consideration. But consider the
> >>> consequences:
> >> Your patch does not fix the problem just try to play them with
> >> ffplay. My patch works, yours does not. That file does have a
> >> sample rate of 44100 not 8000.
> 
> Which makes it a totally invalid encoding.
> 

> >> The easiest solution is simply to remove the code messing with the
> >> sample rate. The alternative would be to add a sample_rate field to
> >> AVStream so both demuxer and decoder can provide their sample rates
> >> to the user app. But this seems really bad design as we know which
> >> rate is wrong and which is not.
> 
> No we don't. A sample rate not 8000 can mean that the user forgot the
> rate conversion *OR* that the user just forgot to store the correct rate
> in the header.

Until we find a file (and not a intentionally created one for this) where
the demuxer says X but the samplerate is not X but 8000 i think the
assumtation that it is always X is pretty good.


> 
> >> And 2 sample rate fields will just force the user app to duplicate 
> >> the logic, the situation would be different if we didnt knew what
> >> was correct but heres its clear, if the demuser says X its X if the
> >> demuxer doesnt say anything its 8000.
> >> 
> >> Most sane would be if(!avctx->sample_rate) avctx->sample_rate=
> >> 8000;
> >> 
> >> IMO
> > 
> > Heres a proper patch for that:
> > 
> 
> Rejected. It would accept, without any warning, completely invalid
> values for encoding.
> 
> Unless, of course, if it's an emergency, in that case do zap all the guards.
> 
> But what we need is an agreed-upon policy. You have stated rather 
> clearly that ffmpeg/ffplay has to be as permissive as possible on 
> decoding; I accept that, but IMNSHO loud warnings are needed.
> 

> On encoding, the most sane is to reject non-conforming parameters. If 

Yes, but its only the samplerate the bitstream is completely conforming
and such bending of the spec is clearly wanted by some people otherwise
such files would not exist. GSM could easily be higher quality per
bitrate than several other codecs.
Also we do have AVCodecContext.strict_std_compliance for the encoder
side.


> codec and muxers disagree, as with MOV wanting to write 13200 in the 
> header, it will be the muxer's job to do the change. Tit for tat.

The bitrate for GSM at 8000hz is 13200 other bitrates are incorrect.
33byte/block / 160samples/block * 8000samples/sec * 8bit/byte = 13200 bit/sec

The bitrate for MS-GSM at 8000hz is 13000 other bitrates are incorrect.
65byte/block / 320samples/block * 8000samples/sec * 8bit/byte = 13000 bit/sec

New patch (which i will apply in 24h) below

Index: libavcodec/libgsm.c
===================================================================
--- libavcodec/libgsm.c	(revision 13005)
+++ libavcodec/libgsm.c	(working copy)
@@ -41,9 +41,18 @@
                avctx->channels);
         return -1;
     }
+
+    if(avctx->codec->decode){
+        if(!avctx->channels)
+            avctx->channels= 1;
+
+        if(!avctx->sample_rate)
+            avctx->sample_rate= 8000;
+    }else{
     if (avctx->sample_rate != 8000) {
         av_log(avctx, AV_LOG_ERROR, "Sample rate 8000Hz required for GSM, got %dHz\n",
                avctx->sample_rate);
+        if(avctx->strict_std_compliance > FF_COMPLIANCE_INOFFICIAL)
         return -1;
     }
     if (avctx->bit_rate != 13000 /* Official */ &&
@@ -51,8 +60,10 @@
         avctx->bit_rate != 0 /* Unknown; a.o. mov does not set bitrate when decoding */ ) {
         av_log(avctx, AV_LOG_ERROR, "Bitrate 13000bps required for GSM, got %dbps\n",
                avctx->bit_rate);
+        if(avctx->strict_std_compliance > FF_COMPLIANCE_INOFFICIAL)
         return -1;
     }
+    }
 
     avctx->priv_data = gsm_create();

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080429/ffcad01c/attachment.pgp>