[FFmpeg-devel] [PATCH] make packet_size in AVFormatContext unsigned

[Ronald S. Bultje]

Hi,

On Mon, Jun 15, 2009 at 4:10 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
> the patch is changing a field to unsiged without you explaining why
> or that you verified that all uses of the field are ok with the changed
> type. Rather it looks like you expect me to find out if your code is
> buggy or not.

Wait, you just (2 months ago) said you'd review it yourself since you
didn't trust my review.

My review is simple (I thought I included the summarized version of
this in my original email, but apparently you want more, so here we
go):
- it's used in options.c as option declaration for ffplay/ffmpeg/etc
- it's used in mpegenc.c to set the muxer packet size. There's a
MpegEncContext->packet_size (signed) duplicate value. As I said in my
original email, I think allowing any value is buggy, you don't want
2GB (or negative) values. You want a value between 0 and a reasonably
large limit (e.g. a few MB), and error out otherwise. In that case,
either signed or unsigned is ok. Right now, the code is buggy, and my
code currently introduces a different bug. I stated this in my
original email also.
- it's used in qcp.c as a get_le16() recipient, so that always fit
either signed/unsigned and thus either signed or unsigned is safe

I tested the above by renaming packet_size to _packet_size and
compiling and looking at all compile failures. There was nothing
except for the above.

Then, why would I like it: I want to get rid of all local packet_size
variables in Muxer/DemuxerContexts (grep for it; fixing each of these
would then be a separate patch). In this particular case, it allows
for a nice optimization since I can set AVFormatContext->packet_size
for ASF and then use that value to allocate the packet size of the
dyn_buf in rtp_asf.c. This isn't necessary, but it's a nice
optimization that prevents multiple realloc()s when reading packets,
since we know the maxpktsize from the header already. (And of course,
if the packet turns out larger because of a broken stream, it would
still work, but then the optimization would no longer apply.)

Ronald