On mar, mag 27, 2014 at 12:12:26 -0400, Vittorio Giovara wrote: > On Tue, May 27, 2014 at 3:24 AM, Alessandro Ghedini > <[email protected]> wrote: > > Sooo, thanks to Anton I've been able to make this work. I went ahead and > > split > > the changes in smaller commits (I cherry-picked the commits from ffmpeg > > directly), and while I've been able to squash a few commits together, there > > are > > still 27 separate patches. > > Neat, thanks for doing this. > > > Now what? I didn't send the patches yet to avoid spamming the list with so > > many > > mails, but I can go ahead and do it if you are ok with it. In any case you > > can > > see all the commits at [0]. To merge in a local branch do: > > I took a look at the set, I only have a few minor suggestions. > > - In some commits there are clauses like "Fixes ticket #715." or > requests for comments that may be removed; perhaps this can be done > when we push them.
Fixed, I think...
> - The commit message and the error message seem unrelated
> 7e5261b222fed04523f2bb31557e5f48ff4e1561.
Fixed (I split the commit in two).
> - 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.
> 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).
> - I would squash 2804a7617a9339802de70bc8a5d6bca60f909427 onto
> /cb03bfba9bfdee67ac40326cd5f18531c1e78fcd
Done.
> - 8138e09f2c8cd9137f1786f306f786719793895e could use a K&R :)
Are you sure? I can't see anything that breaks style rules in that commit.
> - 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).
> - I would squash 9e4b6291397c9a8848129af86f59870fb1230bb6 onto
> 647d7d41c69083bc0fae11d487a07cb06e2016fa
Done.
> - 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.
> Finally I think it's also fine to remove the micro version bumps that
> took place in same changesets.
Done.
> > There may still be some cleaning work to do. Also, the test (make
> > fate-lavf-gif)
> > doesn't work yet, I'll need to look into it.
>
> The changesets modified the gif test results, I am not sure if that
> has any impact.
I just noticed that tests/data/fate/lavf-gif.err contains this:
[gif @ 0x93c2a0] Specified pix_fmt is not supported
in tests/lavf-regression.sh avconv is called with "-pix_fmt rgb24" and that
fails. ffmpeg does the same though, and it works fine there.
Another thing that doesn't work is the pal8 pixel format, but this seems to be
caused by the auto-inserted vf_scale filter.
Cheers
signature.asc
Description: Digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
