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

[Michael Niedermayer]

On Mon, Jun 15, 2009 at 04:39:14PM -0400, Ronald S. Bultje wrote:
> 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.

i dont trust it but i wont review a patch that the author didnt bother to
check for being correct


> 
> 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.

yes, you say your code introduces a bug ...


> - 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.)

i cant follow your reasoning or how any of this is related to the type

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090615/b98d5473/attachment.pgp>