[Ffmpeg-devel] [PATCH] MS-GSM support: draft for review

[Michael Niedermayer]

Hi

On Thu, Nov 09, 2006 at 10:56:45AM +0100, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Wed, Nov 08, 2006 at 11:58:55AM +0100, Michel Bardiaux wrote:
> >>Michael Niedermayer wrote:
> >>>Hi
> >>>
> >>>On Tue, Nov 07, 2006 at 04:47:52PM +0100, Michel Bardiaux wrote:
> >>>[...]
> >>[snip]
> >>>codec_tag could be used ... but this is getting more messy then i hoped,
> >>>i thought we just have to split packets up / merge them :(((
> >>>
> >>>so after reconsidering all that mess, lets go back to your patch
> >>>IMHO the formats are sufficiently different to have individual codec_ids
> >>>bitstream filters to convert still would be welcome of course
> >>>
> >>>could you resubmit your patch without ANY renamings and cosmetics?
> >>>
> >>I think I've removed all the cosmetics and renamings, and even reordered 
> >>the code to make the patch more readable (adds rather than changes).
> >>
> >>I've limited my change to the displaying of the bitrate to the minimum 
> >>so that ffmpeg produces correct messages for both gsm flavors.
> >
> >[...]
> >
> >>Index: libavcodec/libgsm.c
> >>===================================================================
> >>--- libavcodec/libgsm.c	(revision 6940)
> >>+++ libavcodec/libgsm.c	(working copy)
> >>@@ -29,6 +29,7 @@
> >> 
> >> // gsm.h miss some essential constants
> >> #define GSM_BLOCK_SIZE 33
> >>+#define MSGSM_BLOCK_SIZE 65
> >> #define GSM_FRAME_SIZE 160
> >> 
> >> static int libgsm_init(AVCodecContext *avctx) {
> >>@@ -73,6 +74,46 @@
> >>     libgsm_close,
> >> };
> >> 
> >>+static int msgsm_init(AVCodecContext *avctx) {
> >>+    int one = 1;
> >>+
> >>+    if (avctx->channels > 1 || avctx->sample_rate != 8000)
> >>+        return -1;
> >>+
> >>+    avctx->frame_size = 2*GSM_FRAME_SIZE;
> >>+    avctx->block_align = MSGSM_BLOCK_SIZE;
> >>+
> >>+    avctx->priv_data = gsm_create();
> >>+
> >>+    gsm_option(avctx->priv_data, GSM_OPT_WAV49, &one);
> >>+
> >>+    avctx->coded_frame= avcodec_alloc_frame();
> >>+    avctx->coded_frame->key_frame= 1;
> >>+
> >>+    return 0;
> >>+}
> >
> >why duplicate libgsm_init and not merge it like:
> >
> >static int libgsm_init(AVCodecContext *avctx) {
> >    if (avctx->channels > 1 || avctx->sample_rate != 8000)
> >        return -1;
> >
> >    avctx->priv_data = gsm_create();
> >
> >    if(avctx->codec_id == CODEC_ID_MSGSM){
> >        int one = 1;
> >        avctx->frame_size = 2*GSM_FRAME_SIZE;
> >        avctx->block_align = MSGSM_BLOCK_SIZE;
> >        gsm_option(avctx->priv_data, GSM_OPT_WAV49, &one);
> >    }else{
> >        avctx->frame_size = GSM_FRAME_SIZE;
> >        avctx->block_align = GSM_BLOCK_SIZE;
> >    }
> >
> >    avctx->coded_frame= avcodec_alloc_frame();
> >    avctx->coded_frame->key_frame= 1;
> >
> >    return 0;
> >}
> >
> >its not hackish or anything or?
> 
> No, but it would mean having the MS and toast variants under the same 
> configure option --xable-libgsm. I was planning to make them separate 
> configure options in a subsequent patch, 

every codec has its own
--enable/disable automatically
--enable-libgsm just enables/disabled if libgsm is linked


> and maybe rename libgsm (which 
> unfortunately uses the codec-string "gsm"...) to toastgsm in yet another 
> patch.
> 
> Admittedly, the amount of code space saved by having one and not the 
> other would not be big, but you seem to work by the principle "There 
> aint no little profit".

most people will either want both or none, and the way you want to
implement it duplicates all the functions of the codec, and
saving a few bytes in a very common case is better then saving a few
in a rare case
but you can add #ifdefs around the msgsm specific parts of the functons
if you want which would combine the advantages of both, though it would
make the code less readable

also code duplication has not been accpeted in the past and will not
be accpeted in the future, reasons being maintainability, object and
source size and the issue with forgeting to change one of the duplicated
pieces if changing them becomes needed be it a bugfix or otherwise


[...]
> >[...]
> >>Index: libavcodec/avcodec.h
> >>===================================================================
> >>--- libavcodec/avcodec.h	(revision 6940)
> >>+++ libavcodec/avcodec.h	(working copy)
> >>@@ -241,6 +241,8 @@
> >> 
> >>     CODEC_ID_MPEG2TS= 0x20000, /* _FAKE_ codec to indicate a raw MPEG2 
> >>     transport
> >>                          stream (only used by libavformat) */
> >>+
> >>+    CODEC_ID_MSGSM, /* As found in WAV */
> >
> >as ive alraedy said this belongs to the audio codecs, the spot under
> >CODEC_ID_IMC seems the correct one, please put it rather there
> 
> I had, and Benjamin Larsson recommended (on -user) to put it at the end 
> of the enum to preserve binary compatibility.

put it after CODEC_ID_IMC to preserve both binary compatibility and the 
seperation of various codec types


[...]
> >
> >
> >[...]
> >>@@ -323,6 +326,8 @@
> >>         enc->codec_id == CODEC_ID_PCM_S32LE ||
> >>         enc->codec_id == CODEC_ID_PCM_S16LE) {
> >>         bytespersec = enc->sample_rate * blkalign;
> >>+    } else if(enc->codec_id == CODEC_ID_MSGSM) {
> >>+        bytespersec = 13000 / 8;
> >
> >didnt you rather want to fix enc->bit_rate? if no then i would rather
> >leave this out until someone does
> 
> Yes, I would, but neither you nor I have yet come with an architecture 
> to do that cleanly (thread "Audio bitrate"). That value above for 
> bytespersec is *mandated* for msgsm in wav. Any other value, neither WMP 
> nor winamp will play the wav!

setting bit_rate in the init function of the gsm encoder should work, 
adding a list of valid bitrates to AVCodec will work too, and letting
the gsm encoder simply fail with an error message if the bitrate is
invalid is the third option

adding a special case for msgsm to every muxer is not an option


> 
> >
> >
> >>     } else {
> >>         bytespersec = enc->bit_rate / 8;
> >>     }
> >[...]
> >>Index: libavformat/wav.c
> >>===================================================================
> >>--- libavformat/wav.c	(revision 6940)
> >>+++ libavformat/wav.c	(working copy)
> >[...]
> >>@@ -46,6 +46,12 @@
> >>     }
> >>     end_tag(pb, fmt);
> >> 
> >>+    if(s->streams[0]->codec->codec_id == CODEC_ID_MSGSM) {
> >>+        fact = start_tag(pb, "fact");
> >>+        put_le32(pb, 0);
> >>+        end_tag(pb, fact);
> >>+    }
> >
> >interresting, accoridng to microsofts excelent and unambigous documentation
> >(not kidding ...)
> >     Fact Chunk
> >This chunk is required for all WAVE formats other than WAVE_FORMAT_PCM. It 
> >stores file
> >dependent information about the contents of the WAVE data. It currently 
> >specifies the time length of the
> >data in samples.
> >
> >so this must not be under CODEC_ID_MSGSM, also it must be a seperate patch
> >as its not CODEC_ID_MSGSM specific
> 
> I didnt because I dont have any other reference wav except PCM or MSGSM, 
> and I dont like to introduce things that I cant test. And the fact chunk 
> is certainly required for msgsm, so it has to be a *preliminary* patch, 
> otherwise there will be an svn with an incorrect msgsm.

the addition of the fact chunk has to be done before msgsm than

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is