On Thu, May 29, 2014 at 9:39 AM, Alessandro Ghedini
<[email protected]> wrote:
>> - The commit message does not fully explain the required changes
>> 09ab9e3b43d34776f012d0be3d07913b8e24aaf5 besides claiming that's
>> better, faster and saves the world from the evil.
>
> Well, there's lots of code removed, so that would make it "saner and simpler",
> but it doesn't seem to mention it being faster.
I was just exaggerating to point the fact that there is little
technical explanation for such a big change.
>> Perhaps it could be split in small chunks but I am not sure how...
>
> TBQH I wouldn't know where to start.
>
>> Also I don't get the removal of { AV_CODEC_ID_GIF, "gif" }.
>
> I asked Clément (the author of the commit) and this is what he said:
>
> On Thu, May 29, 2014 at 11:51:15AM +0200, Clément Bœsch wrote:
>> Because you don't want to mux the gif into image2 since it's not a
>> animated gif muxer. See f70db22999d713da3306bf29ec763d670b9bf1ea if you
>> want to support standalone gif images as well. And you don't need image2
>> for the gif demuxing either since there is a dedicated gif demuxer that
>> allows to play animated gif.
>
> I think that other commit can be cherry-picked separately (but I haven't tried
> yet).
There you go, you could split this change for starters :)
>> - 8138e09f2c8cd9137f1786f306f786719793895e could use a K&R :)
>
> Are you sure? I can't see anything that breaks style rules in that commit.
yesiamsurethereisnospacewhenmultiplying, but I guess the whole file
can be K&R'd at a later stage, so never mind
>> - I would love to reproduce this
>> 899b56b054681d594c3ea74c6ea44445048bd9b6, also shouldn't the check for
>> dimensions be in the same place as
>> b7513e42976e2003cd3eb4e824fa1d60f96c5ba4
>
> I think those are separate issues, so different commits make sense (e.g. in
> case
> of problems one could revert one change but not the other).
Two commits are fine I was just wondering why we would check for size
overflow in one function and size underflow in another.
>> - 9422d6036079120b1e435e6c711ce9734b0a0995 what issues exactly? Were
>> you able to reproduce any?
>
> I think this is more of a "future-proof" kind of change. That is, if one of
> the
> other headers will be changed to include "put_bits.h" (even indirectly), it
> would probably cause problems without this patch.
Not sure, maybe we could hold this one off for a while?
Feel free to send the whole set, so that a full review may start :)
--
Vittorio
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
