[MPlayer-dev-eng] patch: can't display mp3
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Thu Jan 6 22:34:09 CET 2005
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.
More information about the MPlayer-dev-eng
mailing list