[FFmpeg-devel] [RFC] Chromium FFmpeg patches

[Reimar Döffinger]

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.

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.