[FFmpeg-devel] [PATCHv3 3/6] dvbsubdec: always return the whole buffer as consumed bytes

Marton Balint cus at passwd.hu
Wed Oct 16 21:18:21 CEST 2013

On Wed, 16 Oct 2013, Reimar Döffinger wrote:

> On Wed, Oct 16, 2013 at 07:54:23AM +0100, JULIAN GARDNER wrote:
>>>>>  Unfortunately I don't have a sample transport stream. The only
>>> reference I could find why leftover junk bytes are even possible is here:
>>>>>  http://ffmpeg.org/pipermail/ffmpeg-devel/2011-February/103659.html
>>>>>  (mplayer ts demuxer outputs an extra byte)
>>>>>  The patch is needed to ensure that leftover bytes still do not cause
>>> any frame decode errors after extending avcodec_decode_subtitle2 to support
>>> multiple frames in a single packet. So it's for compatibility and keeping
>>> existing behaviour.
>>>>>  Regards,
>>>>>  Marton
>>>>>  _______________________________________________
>>>>>  ffmpeg-devel mailing list
>>>>>  ffmpeg-devel at ffmpeg.org
>>>>>  http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>  Well I would say "get mplayers demuxer fixed 1st"
>>> Yes, sure, but are you saying that we should change the behaviour of the
>>> decoder to return AVERROR_INVALIDDATA in case of leftover bytes?
>> but you are changing it, instead of returning the correct number of bytes your returning the number of bytes passed to it, which is the wrong number hence the warning message. Who is to say that these bytes do not have some meaning? I know your trying to fix a bug in mplayer, but why not push them for a fix first.
> I don't think he tries to fix a MPlayer bug but just tries to not change
> behaviour more than necessary.
> If for no other reason that it might affect more cases.
> We definitely should not return AVERROR_INVALIDDATA (except with high
> values of strict or similar), we generally should try to accept any
> input we can make sense of.
> If we are confident that leftover bytes are due to a bug somewhere, I
> think printing a warning message (instead of just debug) each time is
> quite ok for example.
> Either way I have a bit of a problem with saying "the correct number of
> bytes", because I think we have quite different behaviours in codecs,
> so "correct" is unfortunately not that well defined IMHO.

Yes, exactly. The whole purpose of the patch is not to fix an mplayer bug, 
but to ensure that dvbsub decoding will not return an error, or cause an 
infinite loop in the user programs after we change the API.

Because after we change the API to support CODEC_CAP_SUBFRAMES and tell 
users to re-feed the leftover bytes to the decode function if there are 
any, consuming 0 bytes and returning no frames is a dangerous combination, 
and likely to cause infinite loops at the users. Therefore I would like to 
eliminate the possibility of that. Even if the leftover bytes generate an 
error in the decode function, and not 0 is returned with no decoded 
frames, that error is generated for packets that were previously OK.

Yes, we change behaviour a bit by returning more bytes than we can parse, 
but I beleive it will cause way less problems than the scenarios I 
described above.


More information about the ffmpeg-devel mailing list