[MPlayer-dev-eng] [PATCH] replacement for internal mpg123 fork (mp3lib)

[Uoti Urpala]

On Wed, 2010-05-12 at 16:44 +0200, Thomas Orgis wrote:
> I incorporated the last mentioned alignments... and also added some

A couple more comments:

+    sh->context = malloc(sizeof(struct ad_mpg123_context));
+    /* Shall I check for NULL here? The struct is smaller than the
+     * error handling code... */

No need to check. Generally MPlayer can't be expected to behave sanely
if a malloc fails, and this doesn't look likely to cause security
problems either (no writes with large offsets to the NULL value that
would corrupt memory instead of segfaulting).


+    /* Old headers don't know MPG123_SEEKBUFFER yet, so use the plain 0x100. */
+    mpg123_param(con->handle, MPG123_ADD_FLAGS, 0x100, 0.0);

This looks like a bad idea. How old are the versions affected? If it's
needed then a version #ifdef around a call using the symbolic name would
be preferable.


+static void update_info(sh_audio_t * sh)
+{
+    struct ad_mpg123_context *con = sh->context;
+    if (con->vbr && --con->delay < 1) {
+        struct mpg123_frameinfo finfo;
+        if (MPG123_OK == mpg123_info(con->handle, &finfo)) {
+            /* Yay for overflow. */
+            if (++con->mean_count < 1)
+                con->mean_count = 2;

First, I'm not sure whether this function is needed at all. The value of
i_bps doesn't matter in most cases, and it's only printed at startup
(before any later updates). It can be used for audio timing, but using
the timing mode which avoids that would be preferable. Assuming it's
kept, the overflow handling seems suboptimal (why set it to 2 instead of
a large value?).


+    case ADCTRL_RESYNC_STREAM:
+        {
+            /* Close/reopen the stream for mpg123 to make sure it doesn't

No need for the brace and extra indent level.


+    case ADCTRL_SKIP_FRAME:

I think this could be dropped (just don't support the ADCTRL_SKIP_FRAME
feature). The whole feature is questionable; decoding and discarding
instead of using this control() functionality would usually be better.