[FFmpeg-devel] [PATCH] avcodec/utils: fix check for invalid timebase when decoding subtitles

[Andreas Cadhalpun]

On 27.12.2015 20:43, Hendrik Leppkes wrote:
> On Sun, Dec 27, 2015 at 8:29 PM, Andreas Cadhalpun
> <andreas.cadhalpun at googlemail.com> wrote:
>> On 27.12.2015 20:10, Hendrik Leppkes wrote:
>>> Invalid timebases have a zero numerator, not denominator.
>>
>> A timebase with zero numerator is probably invalid, but a timebase
>> with zero denominator is not even well defined.
>> So this comment doesn't seem quite right.
>>
>>> Fixes a integer divison by zero.
>>> ---
>>>  libavcodec/utils.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index 19f3f0a..33295ed 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -2435,7 +2435,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
>>>          } else {
>>>              avctx->internal->pkt = &pkt_recoded;
>>>
>>> -            if (avctx->pkt_timebase.den && avpkt->pts != AV_NOPTS_VALUE)
>>> +            if (avctx->pkt_timebase.num && avpkt->pts != AV_NOPTS_VALUE)
>>>                  sub->pts = av_rescale_q(avpkt->pts,
>>>                                          avctx->pkt_timebase, AV_TIME_BASE_Q);
>>>              ret = avctx->codec->decode(avctx, sub, got_sub_ptr, &pkt_recoded);
>>>
>>
>> If avctx->pkt_timebase.den is 0, calling av_rescale_q here will trigger the
>> av_assert2(c > 0) in av_rescale_rnd, so removing this check isn't good.
>>
>> Why not check for both num and den?
>>
> 
> We never check both, invalid timebase is {0, 1}, anything else needs
> to have values in both.

I'd call this unknown timebase, as {0, 1} is the initialization value.
A {1, 0} timebase is certainly not valid.

> All timebase checks are for num only.

The check here was for den and there is a similar check in teletext_decode_frame.

> Its an assert2 for a reason (ie. a developer tool), its checked in an
> error condition right after and returns INT64_MIN.

It's an assert, because it should never happen.

If the check for den here was intended to prevent triggering such an assert,
then it would still be needed.
If on the other hand this check was meant to check for an uninitialized
pkt_timebase, then removing it would be fine.

So you can remove this check, if you're sure it's the second case.
But please clarify this in the commit message.

Best regards,
Andreas