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

[Yasuhiro Matsumoto]

Attila Kinali -
> > > > Ok, please check this.
> > >
> > >--- libmpdemux/demuxer.c.orig	2004-11-26 07:24:00.000000000 +0900
> > >+++ libmpdemux/demuxer.c	2004-12-20 09:19:22.000000000 +0900
> > >@@ -21,6 +21,15 @@
> > >+iconv_string(iconv_t fd, char *str)
> > >+{
> > >
> > >You should add here a documentation entry, see
> > >DOCS/tech/code-documentation.txt
> >
> > ok, i added few comments.
> > (sorry, i'm not native speaker)
>
>The function itself is still not documented.
>Please read DOCS/tech/code-documentation.txt again.
>Every function has to be documented, what it is doing,
>what its input parameter are, their valid range and
>the output parameter.

ok, i added doxygen's comment.

>And while you are at it, please also read DOCS/tech/patches.txt.
>You should not compress patches unless it's really needed.
>It just adds another step for the one reviewing it.

Hmm, i'm at where can't use cvs or smtp from my machine.
i can use http only.
"hotmail" break tab character in attachment text file.
thus i gzipped diff file.
i have better to attach two files ?
(gzipped and not-gzipped)

> > >[...]
> > >+		if (len == 0 || errno == E2BIG)
> > >+		{
> > >+			len = len + fromlen * 2 + 40;
> > >
> > >Where do these values come from ?
> >
> > see the comment.
>
>I rather asked why you choose *2+40, ie is there a
>special reason to double and add 40 to it ?
>
>Beside, you are allocating more space on every round,
>which can make the buffer quite big. You should rather
>check whether you need more space or not.

Ahh, i understand.
this is suitable value which i think.
i changed it to use max of multibyte (6).

> > >+			p = (char*)malloc((unsigned)len);
> > >+			if (p != NULL && done > 0)
> > >+				memcpy(p, result, done);
> > >+			free(result);
> > >+			result = p;
> > >+			if (result == NULL)
> > >+				break;
> > >+		}
> > >
> > >A realloc() would be cleaner here
> >
> > hmmm, i don't understand this.
> > remalloc(NULL, size) won't work on some environment.
> > and, we should write "malloc" and "realloc". and checking fail.
> > my part won't leak when fail of malloc().
>
>
>if(p)
>	realloc(...)
>else
>	malloc(...)
>
>I also dont see how this should leak.

ok, i changed as your said.
however,
realloc() don't free original pointer when alloc was failed.

char *oldp = p;
if (p)
	p = realloc(p, ...);
else
	p = malloc(...);
if (oldp) {
	free(oldp);
}

Thanks for your advices.

- Yasuhiro

-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer_mp3_i18n.diff.gz
Type: application/x-gzip-compressed
Size: 1582 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20041228/2cef1edb/attachment.bin>