diff mbox

[FFmpeg-devel,1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0

Message ID 20180704070523.2573-1-thomas@gllm.fr
State New
Headers show

Commit Message

Thomas Guillem July 4, 2018, 7:05 a.m. UTC 
On macOS, a zero rc_max_rate cause an error from
VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).

on iOS (depending on device/version), a zero rc_max_rate cause invalid
arguments from the vtenc_output_callback after few frames and then a crash
within the VideoToolbox library.
---
 libavcodec/videotoolboxenc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Thomas Guillem July 4, 2018, 11:44 a.m. UTC | #1 
On Wed, Jul 4, 2018, at 09:05, Thomas Guillem wrote:
> On macOS, a zero rc_max_rate cause an error from
> VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).
> 
> on iOS (depending on device/version), a zero rc_max_rate cause invalid
> arguments from the vtenc_output_callback after few frames and then a crash
> within the VideoToolbox library.

In fact, when setting a correct max_rate on iOS, you could still get random crashes the same way. It's happening on ios 11.4 but seems to be OK on iOS 12 Beta.

> ---
>  libavcodec/videotoolboxenc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> index ac847358ab..aa9aae7e05 100644
> --- a/libavcodec/videotoolboxenc.c
> +++ b/libavcodec/videotoolboxenc.c
> @@ -1019,6 +1019,7 @@ static int vtenc_create_encoder(AVCodecContext   *avctx,
>  
>      if (vtctx->codec_id == AV_CODEC_ID_H264) {
>          // kVTCompressionPropertyKey_DataRateLimits is not available 
> for HEVC
> +        if (max_rate > 0) {
>          bytes_per_second_value = max_rate >> 3;
>          bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
>                                            kCFNumberSInt64Type,
> @@ -1058,6 +1059,7 @@ static int vtenc_create_encoder(AVCodecContext   
> *avctx,
>              av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate 
> property: %d\n", status);
>              return AVERROR_EXTERNAL;
>          }
> +        }
>  
>          if (profile_level) {
>              status = VTSessionSetProperty(vtctx->session,
> -- 
> 2.18.0
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Aman Karmani July 5, 2018, 6:49 p.m. UTC | #2 
On Wed, Jul 4, 2018 at 3:05 AM Thomas Guillem <thomas@gllm.fr> wrote:

> On macOS, a zero rc_max_rate cause an error from
> VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).
>
> on iOS (depending on device/version), a zero rc_max_rate cause invalid
> arguments from the vtenc_output_callback after few frames and then a crash
> within the VideoToolbox library.
> ---
>  libavcodec/videotoolboxenc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> index ac847358ab..aa9aae7e05 100644
> --- a/libavcodec/videotoolboxenc.c
> +++ b/libavcodec/videotoolboxenc.c
> @@ -1019,6 +1019,7 @@ static int vtenc_create_encoder(AVCodecContext
>  *avctx,
>
>      if (vtctx->codec_id == AV_CODEC_ID_H264) {
>          // kVTCompressionPropertyKey_DataRateLimits is not available for
> HEVC
> +        if (max_rate > 0) {


I think it would be better to add this condition to the existing if block
above so we can avoid another level of indentation.

Patch looks fine to me otherwise. I can make this change and commit this
week.

Aman


>          bytes_per_second_value = max_rate >> 3;
>          bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
>                                            kCFNumberSInt64Type,
> @@ -1058,6 +1059,7 @@ static int vtenc_create_encoder(AVCodecContext
>  *avctx,
>              av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate
> property: %d\n", status);
>              return AVERROR_EXTERNAL;
>          }
> +        }
>
>          if (profile_level) {
>              status = VTSessionSetProperty(vtctx->session,
> --
> 2.18.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Kári Helgason July 6, 2018, 7:21 a.m. UTC | #3 
Sorry for jumping in with a slightly OT question,

Am I understanding correctly that you have noticed that when using kVTCompre
ssionPropertyKey_DataRateLimits you will sometimes get random garbage from
the encoder in the output callback?

How frequently does this happen, and can you reproduce it reliably, or do
you only see it in production?

Has a radar been filed with Apple about this?

On Wed, Jul 4, 2018 at 1:44 PM Thomas Guillem <thomas@gllm.fr> wrote:

> On Wed, Jul 4, 2018, at 09:05, Thomas Guillem wrote:
> > On macOS, a zero rc_max_rate cause an error from
> > VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).
> >
> > on iOS (depending on device/version), a zero rc_max_rate cause invalid
> > arguments from the vtenc_output_callback after few frames and then a
> crash
> > within the VideoToolbox library.
>
> In fact, when setting a correct max_rate on iOS, you could still get
> random crashes the same way. It's happening on ios 11.4 but seems to be OK
> on iOS 12 Beta.
>
> > ---
> >  libavcodec/videotoolboxenc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> > index ac847358ab..aa9aae7e05 100644
> > --- a/libavcodec/videotoolboxenc.c
> > +++ b/libavcodec/videotoolboxenc.c
> > @@ -1019,6 +1019,7 @@ static int vtenc_create_encoder(AVCodecContext
>  *avctx,
> >
> >      if (vtctx->codec_id == AV_CODEC_ID_H264) {
> >          // kVTCompressionPropertyKey_DataRateLimits is not available
> > for HEVC
> > +        if (max_rate > 0) {
> >          bytes_per_second_value = max_rate >> 3;
> >          bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
> >                                            kCFNumberSInt64Type,
> > @@ -1058,6 +1059,7 @@ static int vtenc_create_encoder(AVCodecContext
> > *avctx,
> >              av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate
> > property: %d\n", status);
> >              return AVERROR_EXTERNAL;
> >          }
> > +        }
> >
> >          if (profile_level) {
> >              status = VTSessionSetProperty(vtctx->session,
> > --
> > 2.18.0
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Thomas Guillem July 7, 2018, 5:54 a.m. UTC | #4 
On Thu, Jul 5, 2018, at 20:49, Aman Gupta wrote:
> On Wed, Jul 4, 2018 at 3:05 AM Thomas Guillem <thomas@gllm.fr> wrote:
> 
> > On macOS, a zero rc_max_rate cause an error from
> > VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).
> >
> > on iOS (depending on device/version), a zero rc_max_rate cause invalid
> > arguments from the vtenc_output_callback after few frames and then a crash
> > within the VideoToolbox library.
> > ---
> >  libavcodec/videotoolboxenc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> > index ac847358ab..aa9aae7e05 100644
> > --- a/libavcodec/videotoolboxenc.c
> > +++ b/libavcodec/videotoolboxenc.c
> > @@ -1019,6 +1019,7 @@ static int vtenc_create_encoder(AVCodecContext
> >  *avctx,
> >
> >      if (vtctx->codec_id == AV_CODEC_ID_H264) {
> >          // kVTCompressionPropertyKey_DataRateLimits is not available for
> > HEVC
> > +        if (max_rate > 0) {
> 
> 
> I think it would be better to add this condition to the existing if block
> above so we can avoid another level of indentation.

That's what I first wanted to do but there is a also a profile level in this code block.

> 
> Patch looks fine to me otherwise. I can make this change and commit this
> week.
> 
> Aman
> 
> 
> >          bytes_per_second_value = max_rate >> 3;
> >          bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
> >                                            kCFNumberSInt64Type,
> > @@ -1058,6 +1059,7 @@ static int vtenc_create_encoder(AVCodecContext
> >  *avctx,
> >              av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate
> > property: %d\n", status);
> >              return AVERROR_EXTERNAL;
> >          }
> > +        }
> >
> >          if (profile_level) {
> >              status = VTSessionSetProperty(vtctx->session,
> > --
> > 2.18.0
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Thomas Guillem July 7, 2018, 5:57 a.m. UTC | #5 
On Fri, Jul 6, 2018, at 09:21, Kári Helgason wrote:
> Sorry for jumping in with a slightly OT question,
> 
> Am I understanding correctly that you have noticed that when using kVTCompre
> ssionPropertyKey_DataRateLimits you will sometimes get random garbage from
> the encoder in the output callback?

Not garbage, but the output callback will be called with a 0 error_status (OK then) but without any sample_buffer.
Then, a crash within the VT lib will follow.

> 
> How frequently does this happen, and can you reproduce it reliably, or do
> you only see it in production?
> 
> Has a radar been filed with Apple about this?

This issue doesn't reproduce on the iOS 12 beta (with the same device), so I guess they are aware or it has been fixed.

> 
> On Wed, Jul 4, 2018 at 1:44 PM Thomas Guillem <thomas@gllm.fr> wrote:
> 
> > On Wed, Jul 4, 2018, at 09:05, Thomas Guillem wrote:
> > > On macOS, a zero rc_max_rate cause an error from
> > > VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).
> > >
> > > on iOS (depending on device/version), a zero rc_max_rate cause invalid
> > > arguments from the vtenc_output_callback after few frames and then a
> > crash
> > > within the VideoToolbox library.
> >
> > In fact, when setting a correct max_rate on iOS, you could still get
> > random crashes the same way. It's happening on ios 11.4 but seems to be OK
> > on iOS 12 Beta.
> >
> > > ---
> > >  libavcodec/videotoolboxenc.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> > > index ac847358ab..aa9aae7e05 100644
> > > --- a/libavcodec/videotoolboxenc.c
> > > +++ b/libavcodec/videotoolboxenc.c
> > > @@ -1019,6 +1019,7 @@ static int vtenc_create_encoder(AVCodecContext
> >  *avctx,
> > >
> > >      if (vtctx->codec_id == AV_CODEC_ID_H264) {
> > >          // kVTCompressionPropertyKey_DataRateLimits is not available
> > > for HEVC
> > > +        if (max_rate > 0) {
> > >          bytes_per_second_value = max_rate >> 3;
> > >          bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
> > >                                            kCFNumberSInt64Type,
> > > @@ -1058,6 +1059,7 @@ static int vtenc_create_encoder(AVCodecContext
> > > *avctx,
> > >              av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate
> > > property: %d\n", status);
> > >              return AVERROR_EXTERNAL;
> > >          }
> > > +        }
> > >
> > >          if (profile_level) {
> > >              status = VTSessionSetProperty(vtctx->session,
> > > --
> > > 2.18.0
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Aman Karmani July 19, 2018, 4:27 p.m. UTC | #6 
On Fri, Jul 6, 2018 at 10:55 PM Thomas Guillem <thomas@gllm.fr> wrote:

>
>
> On Thu, Jul 5, 2018, at 20:49, Aman Gupta wrote:
> > On Wed, Jul 4, 2018 at 3:05 AM Thomas Guillem <thomas@gllm.fr> wrote:
> >
> > > On macOS, a zero rc_max_rate cause an error from
> > > VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).
> > >
> > > on iOS (depending on device/version), a zero rc_max_rate cause invalid
> > > arguments from the vtenc_output_callback after few frames and then a
> crash
> > > within the VideoToolbox library.
> > > ---
> > >  libavcodec/videotoolboxenc.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/libavcodec/videotoolboxenc.c
> b/libavcodec/videotoolboxenc.c
> > > index ac847358ab..aa9aae7e05 100644
> > > --- a/libavcodec/videotoolboxenc.c
> > > +++ b/libavcodec/videotoolboxenc.c
> > > @@ -1019,6 +1019,7 @@ static int vtenc_create_encoder(AVCodecContext
> > >  *avctx,
> > >
> > >      if (vtctx->codec_id == AV_CODEC_ID_H264) {
> > >          // kVTCompressionPropertyKey_DataRateLimits is not available
> for
> > > HEVC
> > > +        if (max_rate > 0) {
> >
> >
> > I think it would be better to add this condition to the existing if block
> > above so we can avoid another level of indentation.
>
> That's what I first wanted to do but there is a also a profile level in
> this code block.
>

I split the profile block into another conditional, and added max_rate to
this one. Applied to master and release/4.0

Aman


>
> >
> > Patch looks fine to me otherwise. I can make this change and commit this
> > week.
> >
> > Aman
> >
> >
> > >          bytes_per_second_value = max_rate >> 3;
> > >          bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
> > >                                            kCFNumberSInt64Type,
> > > @@ -1058,6 +1059,7 @@ static int vtenc_create_encoder(AVCodecContext
> > >  *avctx,
> > >              av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate
> > > property: %d\n", status);
> > >              return AVERROR_EXTERNAL;
> > >          }
> > > +        }
> > >
> > >          if (profile_level) {
> > >              status = VTSessionSetProperty(vtctx->session,
> > > --
> > > 2.18.0
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Thomas Guillem July 20, 2018, 7:54 a.m. UTC | #7 
On Thu, Jul 19, 2018, at 18:27, Aman Gupta wrote:
> On Fri, Jul 6, 2018 at 10:55 PM Thomas Guillem <thomas@gllm.fr> wrote:
> 
> >
> >
> > On Thu, Jul 5, 2018, at 20:49, Aman Gupta wrote:
> > > On Wed, Jul 4, 2018 at 3:05 AM Thomas Guillem <thomas@gllm.fr> wrote:
> > >
> > > > On macOS, a zero rc_max_rate cause an error from
> > > > VTSessionSetProperty(kVTCompressionPropertyKey_DataRateLimits).
> > > >
> > > > on iOS (depending on device/version), a zero rc_max_rate cause invalid
> > > > arguments from the vtenc_output_callback after few frames and then a
> > crash
> > > > within the VideoToolbox library.
> > > > ---
> > > >  libavcodec/videotoolboxenc.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/libavcodec/videotoolboxenc.c
> > b/libavcodec/videotoolboxenc.c
> > > > index ac847358ab..aa9aae7e05 100644
> > > > --- a/libavcodec/videotoolboxenc.c
> > > > +++ b/libavcodec/videotoolboxenc.c
> > > > @@ -1019,6 +1019,7 @@ static int vtenc_create_encoder(AVCodecContext
> > > >  *avctx,
> > > >
> > > >      if (vtctx->codec_id == AV_CODEC_ID_H264) {
> > > >          // kVTCompressionPropertyKey_DataRateLimits is not available
> > for
> > > > HEVC
> > > > +        if (max_rate > 0) {
> > >
> > >
> > > I think it would be better to add this condition to the existing if block
> > > above so we can avoid another level of indentation.
> >
> > That's what I first wanted to do but there is a also a profile level in
> > this code block.
> >
> 
> I split the profile block into another conditional, and added max_rate to
> this one. Applied to master and release/4.0
> 
> Aman

OK, Thanks!

> 
> 
> >
> > >
> > > Patch looks fine to me otherwise. I can make this change and commit this
> > > week.
> > >
> > > Aman
> > >
> > >
> > > >          bytes_per_second_value = max_rate >> 3;
> > > >          bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
> > > >                                            kCFNumberSInt64Type,
> > > > @@ -1058,6 +1059,7 @@ static int vtenc_create_encoder(AVCodecContext
> > > >  *avctx,
> > > >              av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate
> > > > property: %d\n", status);
> > > >              return AVERROR_EXTERNAL;
> > > >          }
> > > > +        }
> > > >
> > > >          if (profile_level) {
> > > >              status = VTSessionSetProperty(vtctx->session,
> > > > --
> > > > 2.18.0
> > > >
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index ac847358ab..aa9aae7e05 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -1019,6 +1019,7 @@  static int vtenc_create_encoder(AVCodecContext   *avctx,
 
     if (vtctx->codec_id == AV_CODEC_ID_H264) {
         // kVTCompressionPropertyKey_DataRateLimits is not available for HEVC
+        if (max_rate > 0) {
         bytes_per_second_value = max_rate >> 3;
         bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
                                           kCFNumberSInt64Type,
@@ -1058,6 +1059,7 @@  static int vtenc_create_encoder(AVCodecContext   *avctx,
             av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate property: %d\n", status);
             return AVERROR_EXTERNAL;
         }
+        }
 
         if (profile_level) {
             status = VTSessionSetProperty(vtctx->session,