[FFmpeg-devel] [PATCH] lavfi/vf_ass: ignore subtitles decoding errors.

[Clément Bœsch]

On Thu, Feb 14, 2013 at 07:20:19PM +0100, Nicolas George wrote:
> Le sextidi 26 pluviôse, an CCXXI, Clement Boesch a écrit :
> > I thought it wasn't necessary anymore...
> 
> IMHO, checking for bugs is always a good thing.
> 
> > OK for this
> > 
> > Note: I don't think using av_assert1 instead of av_assert0 is really
> > relevant.
> 
> That does probably not much of a difference indeed.
> 
> > As said in the previous mail, I think it's good to allow the subtitles
> > decoders to let the AVSubtitle in an incomplete state and prevent anyone
> > from reading in it using got_sub_ptr. Or said differently, got_sub_ptr=0
> > and sub->num_rects=3 is valid since got_sub_ptr overrules reading anything
> > in sub.
> 
> I disagree. It is the same principle as using av_freep rather than av_free:
> do not let an invalid information hanging somewhere, because even if nobody
> is _supposed_ to access it, somebody will eventually, and debugging a null
> dereference is easier than debugging an invalid dereference.
> 
> AFAIK, all our decoders currently behave correctly according to that check.
> I am pretty certain that fixing one that do not behave correctly would be
> pretty trivial: it is just a matter of moving sub->num_rects = num_rects
> later in the function. May I also remind you that storing anything in the
> rectangles would be a memory leak anyway, so I really see no point at all
> for behaving like that.
> 

As long as you are sure num_rects is reset to 0 in all the decoders in
case of, let's say, memory allocation failure of one new rects, OK. I'm
afraid of some code that might be free-ing the other rects (preventing any
leak) but not setting the num_rects to 0 (along with some other incomplete
fields in AVSubtitle), relying on the fact that it is setting *got_sub_ptr
to 0 (or simply not touching it since it's set to 0 by the caller).

I'm not going to oppose to the patch if it works, but if you're going to
be strict about this, please at least improve the documentation of
avcodec_decode_subtitles* to make sure everyone is expecting the same
thing. We can also use it as a reference in the future when in doubt about
the expected behaviour of a decoder.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130214/3936a5c0/attachment.asc>