[FFmpeg-devel] [RFC] Chromium FFmpeg patches

Alex Converse alex.converse
Sat Aug 29 17:36:47 CEST 2009


On Sat, Aug 29, 2009 at 5:03 AM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
>
> On Sat, Aug 29, 2009 at 12:46:05AM -0400, Alex Converse wrote:
> > Google has a variety of patches to FFmpeg as part of their Chromium project.
> > It may be prudent for the Vorbis and VP3 maintainers to go through them and
> > see what is relevant. If not then at some point in the future I'll probably
> > go through them, clean them up (if necessary) and submit them here.
> >
> > http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/to_upstream/
>
> http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/to_upstream/09_mov_stsz_int_oflow.patch?view=log
> issue is on bugzilla, a nicer/cleaner fix (actually several) is there,
> too.
>

indeed

> http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/to_upstream/10_vorbis_submap_indexes.patch?view=log
> should be compared in simplicity to just adding FFMIN().
> Disadvantage: no error message for invalid values.
> Also mode_setup->mapping needs to be checked where it is assigned (and
> the others marked with "FIXME check" possibly as well (some of it
> actually done in patch 12).
>
> http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/to_upstream/13_ogg_comment_parsing.patch?view=log
> Debug message change should be separate, I already had that in my tree
> in the past once.
> The first check seems plain unnecessary.
> For the other case just changing
> > if (end - p < s)
> to
> > if (p >= end || end - p < s)
> should be enough.
> For consistency, changing the first if of that style to match might make
> sense.
>
> I'll not review the other vorbis stuff, there is just too few checks in
> that code, whoever oks the stuff in the end should have a look instead.
>
> http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/to_upstream/16_fix_div_by_zero1.patch?view=log
> I think we have clarified that we think such checks only hide the real
> issues and the demuxers must validate time bases.
> Though it might make sense to think about factoring out the
> timebase-sanitation code somewhen.
>
> The vp3 stuff (18 and 23) seem ok, too.
>
> The vorbis error checking somehow must be done differently IMO, adding 4
> lines of code in almost any place where we read something as well as
> failing hard each time (particularly since that also means you have to
> double-check this doesn't result in memleaks) isn't ideal in my opinion.
> One possibility might be to read all the setup data and the validate it
> all in one go, though I am not sure how many of the checks that would
> cover at once.

What about 27_vorbis_residue_loop_error.patch? It seems straight
forward but it makes me wonder how the code could have ever worked.

Anything that we don't fix where there is a sample should probably be
imported to roundup and samples, no?



More information about the ffmpeg-devel mailing list