[MPlayer-dev-eng] [PATCH] vf_delogo: variable rectangle

[Nicolas George]

Le primidi 21 pluviôse, an CCXIX, Reimar Döffinger a écrit :
> const char *
> is probably better.

Changed.

> doxy-documentation for new functions would be nice.

Added.

> Seems to me liek you could just do this in fix_band

Indeed.

> Are you sure you want to allow it to go backwards?

Yes. The user may have just hit the left arrow to go back a few seconds.

> This looks a bit double...

Fixed.

> > +            rect = realloc(rect, alloc_rect * sizeof(*rect));
> Potential integer overflow.

Indeed. What is the solution in that case?

	if (alloc_rect * sizeof(*rect) / sizeof(*rect) != alloc_rect)
	    abort();

? There are a lot of similar problems all around the code.

> > +            if (!rect)
> > +                abort();
> Abort sure isn't nice. Haven't looked what if any the alternatives are.

I looked around before writing this code. There is no consistent behaviour
about this: there are a lot of unchecked mallocs, error returns which will
be later ignored, etc.

On current operating systems, mermory is overcommitted and small mallocs
never fail, so trying to recover gracefully on malloc failure in these cases
is mostly wasted time.

I think what we need is something like mp_realloc_or_die that takes care
both of improbable failure and integer overflow.

> I'd expect this whole part can be simplified by fscanf, in the worst case
> by also using getc/ungetc

I do not see it getting simpler than that.

> > +    rect = realloc(rect, n_rect * sizeof(*rect));
> That's just pointless just to save a few bytes IMO.

I find it sloppy not to do it, as it is just a simple line like that.
Furthermore, it would allow valgrind to check out of bounds accesses later
for example.

Again, mp_enlarge_array / mp_finish_array would probably be nice for
similar cases, which are quite common.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vf_delogo-allow-to-change-the-rectangle-based-on-the.patch
Type: text/x-diff
Size: 6842 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110209/9e6b1990/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110209/9e6b1990/attachment.pgp>