[MPlayer-dev-eng] [Suggested PATCH] Increase maximum ftp file path length

[Reimar Döffinger]

On Mon, Nov 05, 2012 at 10:18:47AM +0200, Sergey wrote:
> On Mon, 5 Nov 2012 13:19:19 Xidorn Quan wrote:
> 
> >> -  char rsp_txt[256];
> >> +  char rsp_txt[65536];
> >> -  char str[256],rsp_txt[256];
> >> +  char str[65536],rsp_txt[65536];
> 
> > I don't think it's a good idea to put such a large array in stack.
> 
> Well, it's just I hit 100 characters limitation for UTF8 many times
> on real URLs, so I tried to increase it to something that "must
> definitely be enough". Default stack size is about 10 megabytes, and
> I guess all those functions are leafs, they're neither recursive, nor
> long-lived, thus won't take stack from other parts of mplayer.

I am quite sure a stack size of 8 MB was default not so long ago,
and in some cases it can be a lot less.

> Anyway. What should I do? Should I decrease the size and resuggest
> patch? What is the maximum allowed size then? Is 16384 good?

IMHO anything above 4k is bad practice.

> Or should I mark all these variables "static" instead?

No, that's really bad for maintainability.
I don't really see a reason to not use heap allocation of proper size,
those functions are hardly speed-critical.

> PS: just looked around:
> libavformat/rtsp.c:    char buf[16384], *q;
> libavformat/rtmpproto.c:        uint8_t tmp_buf[16384];
> libavcodec/vorbisenc.c:    uint8_t buffer[50000] = {0}, *p = buffer;
> libmpdemux/demux_audio.c:     unsigned char buf[16384]; // vlc uses 16384*4 (4 dts frames)

That one's just bad code, there is no reason to use a buffer at all.

> libmpdemux/demux_ts.c:        struct {...} pes_priv1[8192], *pptr;

There's a few more in that file, though they use the NB_PID_MAX
define.