[MPlayer-dev-eng] vbr mp3 runtime

[Ingo Brückl]

Reimar Döffinger wrote on Tue, 25 Oct 2011 18:32:04 +0200:

> The point is that movi_end can be 0 (there are quite a few checks for
> that in the code) and then you'll (maybe?) overwrite the bitrate to 0!

Ok, so let's do demux_audio.patch first to ensure, movi_end will be properly
set and only change the bitrate if (duration && demuxer->movi_end).

>> +static unsigned int mp3_vbr_frames(stream_t *s, off_t off) {

> Not much point yet, but in principle I'm in favour of completely
> avoiding off_t, it tends to be 32 bit on MinGW, which means that
> supporting large files without hacking the system headers is basically
> not possible.
> If we had used (u)int64_t everywhere it would just be a matter of using
> the 64 bit functions in stream_file...

I was just using the data type that is used in the demuxer structure as well.
Does it make sense to change to another type here?

>> +  unsigned int data;
>> +  unsigned char hdr[4];

> When you assume a certain number of bits, it's generally nicer
> to use the uint32_t and uint8_t etc. types. It's also less to
> write and doesn't give people really horrible ideas like just using
> "char" when they need a signed type.

Same argument as above: I was just using the data types that the functions
using this variable are expecting. mp_check_mp3_header() requires unsigned
int and mp_get_mp3_header() requires unsigned char*.

>> +  const off_t xing_offt[2][2] = {{32, 17}, {17, 9}};

> off_t is a bit overkill really.
> Also adding "static" will avoid some compilers doing something stupid
> like creating it on stack with every call.

Changed to static and int.

So, may I commit then?

Ingo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: demux_audio.patch
Type: text/x-diff
Size: 800 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20111026/5835735e/attachment.bin>
-------------- next part --------------
--- /tmp/Changelog	2011-10-26 14:44:07.000000000 +0200
+++ Changelog	2011-10-26 14:44:19.000000000 +0200
@@ -8,6 +8,7 @@
 
     Demuxers:
     * experimental support for using binary Quicktime codecs with -demuxer lavf.
+    * correct runtime and average bitrate for VBR (variable bitrate) MP3
 
     Filters:
     * delogo: allow to change the rectangle based on the time.
--- /tmp/demux_audio.c	2011-10-26 14:44:09.000000000 +0200
+++ libmpdemux/demux_audio.c	2011-10-26 15:04:09.000000000 +0200
@@ -260,6 +260,69 @@
 }
 #endif
 
+/**
+ * @brief Determine the number of frames of a file encoded with
+ *        variable bitrate mode (VBR).
+ *
+ * @param s stream to be read
+ * @param off offset in stream to start reading from
+ *
+ * @return 0 (error or no variable bitrate mode) or number of frames
+ */
+static unsigned int mp3_vbr_frames(stream_t *s, off_t off) {
+  static const int xing_offset[2][2] = {{32, 17}, {17, 9}};
+  unsigned int data;
+  unsigned char hdr[4];
+  int framesize, chans, spf, layer;
+
+  if ((s->flags & MP_STREAM_SEEK) == MP_STREAM_SEEK) {
+
+    if (!stream_seek(s, off)) return 0;
+
+    data = stream_read_dword(s);
+    hdr[0] = data >> 24;
+    hdr[1] = data >> 16;
+    hdr[2] = data >> 8;
+    hdr[3] = data;
+
+    if (!mp_check_mp3_header(data)) return 0;
+
+    framesize = mp_get_mp3_header(hdr, &chans, NULL, &spf, &layer, NULL);
+
+    if (framesize == -1 || layer != 3) return 0;
+
+    /* Xing / Info (at variable position: 32, 17 or 9 bytes after header) */
+
+    if (!stream_skip(s, xing_offset[spf < 1152][chans == 1])) return 0;
+
+    data = stream_read_dword(s);
+
+    if (data == MKBETAG('X','i','n','g') || data == MKBETAG('I','n','f','o')) {
+      data = stream_read_dword(s);
+
+      if (data & 0x1)                   // frames field is present
+        return stream_read_dword(s);    // frames
+    }
+
+    /* VBRI (at fixed position: 32 bytes after header) */
+
+    if (!stream_seek(s, off + 4 + 32)) return 0;
+
+    data = stream_read_dword(s);
+
+    if (data == MKBETAG('V','B','R','I')) {
+      data = stream_read_word(s);
+
+      if (data == 1) {                       // check version
+        if (!stream_skip(s, 8)) return 0;    // skip delay, quality and bytes
+        return stream_read_dword(s);         // frames
+      }
+    }
+  }
+
+  return 0;
+}
+
 static int demux_audio_open(demuxer_t* demuxer) {
   stream_t *s;
   sh_audio_t* sh_audio;
@@ -269,6 +332,7 @@
   // mp3_hdrs list is sorted first by next_frame_pos and then by frame_pos
   mp3_hdr_t *mp3_hdrs = NULL, *mp3_found = NULL;
   da_priv_t* priv;
+  double duration;
 
   s = demuxer->stream;
 
@@ -345,7 +409,7 @@
     sh_audio->wf->nBlockAlign = mp3_found->mpa_spf;
     sh_audio->wf->wBitsPerSample = 16;
     sh_audio->wf->cbSize = 0;
-    sh_audio->i_bps = sh_audio->wf->nAvgBytesPerSec;
+    duration = (double) mp3_vbr_frames(s, demuxer->movi_start) * mp3_found->mpa_spf / mp3_found->mp3_freq;
     free(mp3_found);
     mp3_found = NULL;
     if(s->end_pos && (s->flags & MP_STREAM_SEEK) == MP_STREAM_SEEK) {
@@ -381,6 +445,8 @@
 	demux_info_add(demuxer,"Genre",genres[g]);
       }
     }
+    if (duration && demuxer->movi_end) sh_audio->wf->nAvgBytesPerSec = demuxer->movi_end / duration;
+    sh_audio->i_bps = sh_audio->wf->nAvgBytesPerSec;
     break;
   case WAV: {
     unsigned int chunk_type;