diff mbox

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

Message ID 20180703150550.17177-1-thomas@gllm.fr
State Superseded
Headers show

Commit Message

Thomas Guillem July 3, 2018, 3:05 p.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 | 72 ++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

Comments

Carl Eugen Hoyos July 3, 2018, 6:53 p.m. UTC | #1 
2018-07-03 17:05 GMT+02:00, Thomas Guillem <thomas@gllm.fr>:
> 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 | 72 ++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> index ac847358ab..050e5cefee 100644
> --- a/libavcodec/videotoolboxenc.c
> +++ b/libavcodec/videotoolboxenc.c
> @@ -1019,44 +1019,46 @@ static int vtenc_create_encoder(AVCodecContext
> *avctx,
>
>      if (vtctx->codec_id == AV_CODEC_ID_H264) {
>          // kVTCompressionPropertyKey_DataRateLimits is not available for
> HEVC
> -        bytes_per_second_value = max_rate >> 3;
> -        bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
> -                                          kCFNumberSInt64Type,
> -                                          &bytes_per_second_value);
> -        if (!bytes_per_second) {
> -            return AVERROR(ENOMEM);
> -        }
> -        one_second_value = 1;
> -        one_second = CFNumberCreate(kCFAllocatorDefault,
> -                                    kCFNumberSInt64Type,
> -                                    &one_second_value);
> -        if (!one_second) {
> -            CFRelease(bytes_per_second);
> -            return AVERROR(ENOMEM);
> -        }
> -        nums[0] = (void *)bytes_per_second;
> -        nums[1] = (void *)one_second;
> -        data_rate_limits = CFArrayCreate(kCFAllocatorDefault,
> -                                         (const void **)nums,
> -                                         2,
> -                                         &kCFTypeArrayCallBacks);

Please do the re-indentation in a separate patch to make
reviewing your change easier, both now and on the
commit mailing list.

Carl Eugen
Thomas Guillem July 4, 2018, 7:06 a.m. UTC | #2 
On Tue, Jul 3, 2018, at 20:53, Carl Eugen Hoyos wrote:
> 2018-07-03 17:05 GMT+02:00, Thomas Guillem <thomas@gllm.fr>:
> > 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 | 72 ++++++++++++++++++------------------
> >  1 file changed, 37 insertions(+), 35 deletions(-)
> >
> > diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> > index ac847358ab..050e5cefee 100644
> > --- a/libavcodec/videotoolboxenc.c
> > +++ b/libavcodec/videotoolboxenc.c
> > @@ -1019,44 +1019,46 @@ static int vtenc_create_encoder(AVCodecContext
> > *avctx,
> >
> >      if (vtctx->codec_id == AV_CODEC_ID_H264) {
> >          // kVTCompressionPropertyKey_DataRateLimits is not available for
> > HEVC
> > -        bytes_per_second_value = max_rate >> 3;
> > -        bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
> > -                                          kCFNumberSInt64Type,
> > -                                          &bytes_per_second_value);
> > -        if (!bytes_per_second) {
> > -            return AVERROR(ENOMEM);
> > -        }
> > -        one_second_value = 1;
> > -        one_second = CFNumberCreate(kCFAllocatorDefault,
> > -                                    kCFNumberSInt64Type,
> > -                                    &one_second_value);
> > -        if (!one_second) {
> > -            CFRelease(bytes_per_second);
> > -            return AVERROR(ENOMEM);
> > -        }
> > -        nums[0] = (void *)bytes_per_second;
> > -        nums[1] = (void *)one_second;
> > -        data_rate_limits = CFArrayCreate(kCFAllocatorDefault,
> > -                                         (const void **)nums,
> > -                                         2,
> > -                                         &kCFTypeArrayCallBacks);
> 
> Please do the re-indentation in a separate patch to make
> reviewing your change easier, both now and on the
> commit mailing list

OK, done, cf. http://ffmpeg.org/pipermail/ffmpeg-devel/2018-July/231931.html

> 
> Carl Eugen
> _______________________________________________
> 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..050e5cefee 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -1019,44 +1019,46 @@  static int vtenc_create_encoder(AVCodecContext   *avctx,
 
     if (vtctx->codec_id == AV_CODEC_ID_H264) {
         // kVTCompressionPropertyKey_DataRateLimits is not available for HEVC
-        bytes_per_second_value = max_rate >> 3;
-        bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
-                                          kCFNumberSInt64Type,
-                                          &bytes_per_second_value);
-        if (!bytes_per_second) {
-            return AVERROR(ENOMEM);
-        }
-        one_second_value = 1;
-        one_second = CFNumberCreate(kCFAllocatorDefault,
-                                    kCFNumberSInt64Type,
-                                    &one_second_value);
-        if (!one_second) {
-            CFRelease(bytes_per_second);
-            return AVERROR(ENOMEM);
-        }
-        nums[0] = (void *)bytes_per_second;
-        nums[1] = (void *)one_second;
-        data_rate_limits = CFArrayCreate(kCFAllocatorDefault,
-                                         (const void **)nums,
-                                         2,
-                                         &kCFTypeArrayCallBacks);
-
-        if (!data_rate_limits) {
+        if (max_rate > 0) {
+            bytes_per_second_value = max_rate >> 3;
+            bytes_per_second = CFNumberCreate(kCFAllocatorDefault,
+                                              kCFNumberSInt64Type,
+                                              &bytes_per_second_value);
+            if (!bytes_per_second) {
+                return AVERROR(ENOMEM);
+            }
+            one_second_value = 1;
+            one_second = CFNumberCreate(kCFAllocatorDefault,
+                                        kCFNumberSInt64Type,
+                                        &one_second_value);
+            if (!one_second) {
+                CFRelease(bytes_per_second);
+                return AVERROR(ENOMEM);
+            }
+            nums[0] = (void *)bytes_per_second;
+            nums[1] = (void *)one_second;
+            data_rate_limits = CFArrayCreate(kCFAllocatorDefault,
+                                             (const void **)nums,
+                                             2,
+                                             &kCFTypeArrayCallBacks);
+
+            if (!data_rate_limits) {
+                CFRelease(bytes_per_second);
+                CFRelease(one_second);
+                return AVERROR(ENOMEM);
+            }
+            status = VTSessionSetProperty(vtctx->session,
+                                          kVTCompressionPropertyKey_DataRateLimits,
+                                          data_rate_limits);
+
             CFRelease(bytes_per_second);
             CFRelease(one_second);
-            return AVERROR(ENOMEM);
-        }
-        status = VTSessionSetProperty(vtctx->session,
-                                      kVTCompressionPropertyKey_DataRateLimits,
-                                      data_rate_limits);
+            CFRelease(data_rate_limits);
 
-        CFRelease(bytes_per_second);
-        CFRelease(one_second);
-        CFRelease(data_rate_limits);
-
-        if (status) {
-            av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate property: %d\n", status);
-            return AVERROR_EXTERNAL;
+            if (status) {
+                av_log(avctx, AV_LOG_ERROR, "Error setting max bitrate property: %d\n", status);
+                return AVERROR_EXTERNAL;
+            }
         }
 
         if (profile_level) {