libavcodec: Don't crash in avcodec_encode_audio if time_base isn't set

Message ID 1327607384-27968-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Jan. 26, 2012, 7:49 p.m.
Earlier, calling avcodec_encode_audio worked fine even if time_base
wasn't set. Now it crashes due to trying to scale the output pts to
the codec context time base. This affects e.g. VLC.

If no time_base is set, assume it is the same as the sample rate.
This should probably make it behave just as it did before.

CC: libav-stable@libav.org
---
 libavcodec/utils.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

Comments

Martin Storsjö Jan. 26, 2012, 7:55 p.m. | #1
On Thu, 26 Jan 2012, Martin Storsjö wrote:

> Earlier, calling avcodec_encode_audio worked fine even if time_base
> wasn't set. Now it crashes due to trying to scale the output pts to
> the codec context time base. This affects e.g. VLC.
>
> If no time_base is set, assume it is the same as the sample rate.
> This should probably make it behave just as it did before.
>
> CC: libav-stable@libav.org
> ---
> libavcodec/utils.c |    9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 184f67e..5623445 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -1009,9 +1009,12 @@ int attribute_align_arg avcodec_encode_audio(AVCodecContext *avctx,
>         /* fabricate frame pts from sample count.
>            this is needed because the avcodec_encode_audio() API does not have
>            a way for the user to provide pts */
> -        frame->pts = av_rescale_q(avctx->internal->sample_count,
> -                                  (AVRational){ 1, avctx->sample_rate },
> -                                  avctx->time_base);
> +        if (avctx->time_base.den)
> +            frame->pts = av_rescale_q(avctx->internal->sample_count,
> +                                      (AVRational){ 1, avctx->sample_rate },
> +                                      avctx->time_base);
> +        else
> +            frame->pts = avctx->internal->sample_count;
>         avctx->internal->sample_count += frame->nb_samples;
>     } else {
>         frame = NULL;
> -- 
> 1.7.3.1

There's a few other cases of similar scaling in avcodec_encode_audio2, 
too. But I don't think that's as much an issue, since if you port your app 
to avcodec_encode_audio2, you'll notice, while this issue in the fallback 
hits unknowning users when they upgrade to libav 0.8.

// Martin
Justin Ruggles Jan. 26, 2012, 8:38 p.m. | #2
On 01/26/2012 02:55 PM, Martin Storsjö wrote:

> On Thu, 26 Jan 2012, Martin Storsjö wrote:
> 
>> Earlier, calling avcodec_encode_audio worked fine even if time_base
>> wasn't set. Now it crashes due to trying to scale the output pts to
>> the codec context time base. This affects e.g. VLC.
>>
>> If no time_base is set, assume it is the same as the sample rate.
>> This should probably make it behave just as it did before.
>>
>> CC: libav-stable@libav.org
>> ---
>> libavcodec/utils.c |    9 ++++++---
>> 1 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index 184f67e..5623445 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -1009,9 +1009,12 @@ int attribute_align_arg avcodec_encode_audio(AVCodecContext *avctx,
>>         /* fabricate frame pts from sample count.
>>            this is needed because the avcodec_encode_audio() API does not have
>>            a way for the user to provide pts */
>> -        frame->pts = av_rescale_q(avctx->internal->sample_count,
>> -                                  (AVRational){ 1, avctx->sample_rate },
>> -                                  avctx->time_base);
>> +        if (avctx->time_base.den)
>> +            frame->pts = av_rescale_q(avctx->internal->sample_count,
>> +                                      (AVRational){ 1, avctx->sample_rate },
>> +                                      avctx->time_base);
>> +        else
>> +            frame->pts = avctx->internal->sample_count;
>>         avctx->internal->sample_count += frame->nb_samples;
>>     } else {
>>         frame = NULL;
>> -- 
>> 1.7.3.1
> 
> There's a few other cases of similar scaling in avcodec_encode_audio2, 
> too. But I don't think that's as much an issue, since if you port your app 
> to avcodec_encode_audio2, you'll notice, while this issue in the fallback 
> hits unknowning users when they upgrade to libav 0.8.


Why not check in avcodec_open2()?

-Justin
Martin Storsjö Jan. 26, 2012, 8:40 p.m. | #3
On Thu, 26 Jan 2012, Justin Ruggles wrote:

> On 01/26/2012 02:55 PM, Martin Storsjö wrote:
>
>> On Thu, 26 Jan 2012, Martin Storsjö wrote:
>>
>>> Earlier, calling avcodec_encode_audio worked fine even if time_base
>>> wasn't set. Now it crashes due to trying to scale the output pts to
>>> the codec context time base. This affects e.g. VLC.
>>>
>>> If no time_base is set, assume it is the same as the sample rate.
>>> This should probably make it behave just as it did before.
>>>
>>> CC: libav-stable@libav.org
>>> ---
>>> libavcodec/utils.c |    9 ++++++---
>>> 1 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index 184f67e..5623445 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -1009,9 +1009,12 @@ int attribute_align_arg avcodec_encode_audio(AVCodecContext *avctx,
>>>         /* fabricate frame pts from sample count.
>>>            this is needed because the avcodec_encode_audio() API does not have
>>>            a way for the user to provide pts */
>>> -        frame->pts = av_rescale_q(avctx->internal->sample_count,
>>> -                                  (AVRational){ 1, avctx->sample_rate },
>>> -                                  avctx->time_base);
>>> +        if (avctx->time_base.den)
>>> +            frame->pts = av_rescale_q(avctx->internal->sample_count,
>>> +                                      (AVRational){ 1, avctx->sample_rate },
>>> +                                      avctx->time_base);
>>> +        else
>>> +            frame->pts = avctx->internal->sample_count;
>>>         avctx->internal->sample_count += frame->nb_samples;
>>>     } else {
>>>         frame = NULL;
>>> --
>>> 1.7.3.1
>>
>> There's a few other cases of similar scaling in avcodec_encode_audio2,
>> too. But I don't think that's as much an issue, since if you port your app
>> to avcodec_encode_audio2, you'll notice, while this issue in the fallback
>> hits unknowning users when they upgrade to libav 0.8.
>
>
> Why not check in avcodec_open2()?

That would work too. I guess the check would have to be of this form, 
then:

if (codec_type == AUDIO && !time_base.den) {
     time_base.num = 1;
     time_base.den = sample_rate;
}

// Martin

Patch

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 184f67e..5623445 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -1009,9 +1009,12 @@  int attribute_align_arg avcodec_encode_audio(AVCodecContext *avctx,
         /* fabricate frame pts from sample count.
            this is needed because the avcodec_encode_audio() API does not have
            a way for the user to provide pts */
-        frame->pts = av_rescale_q(avctx->internal->sample_count,
-                                  (AVRational){ 1, avctx->sample_rate },
-                                  avctx->time_base);
+        if (avctx->time_base.den)
+            frame->pts = av_rescale_q(avctx->internal->sample_count,
+                                      (AVRational){ 1, avctx->sample_rate },
+                                      avctx->time_base);
+        else
+            frame->pts = avctx->internal->sample_count;
         avctx->internal->sample_count += frame->nb_samples;
     } else {
         frame = NULL;