[FFmpeg-devel] [mov] Fix trampling of ctts during seeks when sidx support is enabled.

[Rodger Combs]

+            sc->ctts_data = av_fast_realloc(sc->ctts_data, &sc->ctts_allocated_size, request_size);
^ this line is incorrect; setting realloc's first arg to its return value leaks the existing allocation in the OOM case. Since you're doing your own calculation for the desired new size here, you may want to use av_reallocp (which frees the original allocation on failure).

When reading a trun that requires this sort of realloc, is it common for there to be any existing entries in the range we're writing? Would it be safe to remove+replace them? Could we do a single realloc + single memmove, and then fill the newly-opened space, rather than repeating the memmove for each entry?

> On Aug 4, 2017, at 18:02, Dale Curtis <dalecurtis at chromium.org> wrote:
> 
> Any comments here? +rodger who wrote the original sidx processing for review.
> 
> - dale
> 
> On Mon, Jul 31, 2017 at 3:01 PM, Dale Curtis <dalecurtis at chromium.org <mailto:dalecurtis at chromium.org>> wrote:
> Whoops, that patch accidentally reverted to an earlier version. Here's the fixed one that works with the mpg sample mentioned above.
> 
> - dale
> 
> On Mon, Jul 31, 2017 at 2:29 PM, Dale Curtis <dalecurtis at chromium.org <mailto:dalecurtis at chromium.org>> wrote:
> Here's an updated patch with a fate test attached. You'll need to add http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 <http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4> to the fate-suite/mov for this test. This is licensed under creative commons attribution, so it should be fine for tests: https://peach.blender.org/about/ <https://peach.blender.org/about/> I tried to use the existing samples, but you need a clip long enough that the entire index isn't generated from the start.
> 
> - dale
> 
> On Tue, Jul 25, 2017 at 1:03 PM, Michael Niedermayer <michael at niedermayer.cc <mailto:michael at niedermayer.cc>> wrote:
> On Mon, Jul 24, 2017 at 02:32:41PM -0700, Dale Curtis wrote:
> > On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer <michael at niedermayer.cc
> > > wrote:
> >
> > > Hi
> > >
> > > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
> > > > Thanks will take a look. Is this test not part of fate? make fate passed
> > >
> > > no, we should have tests for all (fixed) tickets in fate ideally
> > > but in reality most tickets lack a corresponding test
> > > I tried both in outreachy and as well in GSoC to improve this situation
> > > with student projects but both only moved this forward by a small
> > > step. Its a large amount of work to create robust, portable and
> > > practical tests for "all" tickets and everything else.
> > > The way out to get this actually done would be to pay a developer to
> > > create tests for "all" tickets in fate. I belive carl would be the
> > > ideal one to do this work as he has since a very long time always tested
> > > and kept track of all our tickets.
> > > I did suggest a while ago to someone at google that funding such
> > > project would make sense but IIRC i never heared back.
> > > if some company would fund something like this, i belive this would be
> > > very usefull in the long run for code quality
> > >
> >
> > I think it'd be pretty hard to get someone to go through and create tests
> > for every issue ever seen. Even Chromium has trouble with this, but making
> 
> yes, unless theres someone who enjoys doing this kind of work.
> 
> 
> > it part of the culture helps. I.e. reviewers should ask for every change to
> > include a test.
> 
> yes though theres already
> 1.8 patch submission checklist
> ...
> 26. Consider adding a regression test for your code.
> on http://ffmpeg.org/developer.html <http://ffmpeg.org/developer.html>
> 
> 
> > I'm happy to add one to fate for this change. Or can do so
> > in a followup patch if you prefer.
> 
> please do (any variant is fine)
> 
> thx
> 
> >
> > Does anyone have comments on this change specifically? We've already rolled
> > this into Chrome and it's working fine and passing all regression tests.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at ffmpeg.org>
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Breaking DRM is a little like attempting to break through a door even
> though the window is wide open and the only thing in the house is a bunch
> of things you dont want and which you would get tomorrow for free anyway
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at ffmpeg.org>
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> 
> 
>