[MPlayer-dev-eng] patch: can't display mp3

[Reimar Döffinger]

Hi,
On Thu, Jan 06, 2005 at 09:32:10PM +0100, Attila Kinali wrote:
> On Wed, 5 Jan 2005 17:21:25 +0100
> Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> > Those are only cometics. Put the { on a new line and remove the space
> > you added before the mp_msg.
> 
> If this is the only complaint, i'll fix it while committing.
> Any serious problems within the code?

Mostly that it is lots of code for something I find useless ;-)

+static char *
+iconv_string(iconv_t fd, char *str)
+{
+       const char      *from;
+       size_t  fromlen;
+       char    *to;
+       size_t  tolen;
+       size_t  len = 0;
+       size_t  done = 0;
+       char    *result = NULL;
+
+       from = (char *)str;

That extra from variable is not needed and that cast is complete
nonsense. Also str should be const (and actually from should be replaced
by str).

+       fromlen = strlen(from);
+       for (;;)

I loathe that for(;;) style. But if you find it okay...

+               if (len == 0 || errno == E2BIG)
+               {
+               /* Allocate enough room for most conversions.  When
re-allocating
+                * increase the buffer size. */
+                       len = fromlen * 6;      // 6 meam max of
multibyte.
+                       char* result_old = result;

Won't compile on gcc 2.95

[...]
+               to = (char *)result + done;

that typecast doesn't make sense. Also most parts of MPlayer use the
&result[done] syntax...

+               /* Avoid a warning for systems with a wrong iconv()
prototype by
+                * casting the second argument to void *. */
+               if (iconv(fd, (void *)&from, &fromlen, &to, &tolen) !=
(size_t)-1)

Whoever has such a broken prototype should get that warning. I am very
much against working around that.

+               else if (errno != E2BIG)
+               {
+               /* conversion failed */
+                       free(result);
+                       result = NULL;
+                       break;
+               }

I think it should return the original string if it can't convert.

+#ifdef USE_ICONV
+    for(n = 0; info[2*n] != NULL ; n++) {
+      if (info_conv != (iconv_t)-1) {
+        /*  stream info often wrote by other encodings.  */
+        char *ptr = iconv_string(info_conv, info[2*n+1]);
+        mp_msg(MSGT_DEMUX, MSGL_INFO, " %s: %s\n",info[2*n],ptr ? ptr :
info[2*n+1]);
+        if (ptr) free(ptr);
+      }
+      else
+        /* not specified "subcp" option, or unknown encoding name.*/
+#endif
+       mp_msg(MSGT_DEMUX, MSGL_INFO, " %s:
%s\n",info[2*n],info[2*n+1]);
+    }

The number of braces don't match here. I guess the #ifdef must be one
line below.

Apart from that I find it ugly as hell. I think somebody should sleep
about it a night and clean it up. And by cleaning up I mean spending
more than just a few seconds on every line.

Greetings,
Reimar Döffinger

P.S. And please try to send patches as plaintext attachments, it's just
a pain in the ass to review like this.