Re: [FFmpeg-devel] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
On Thu, Jul 19, 2018, at 18:27, Aman Gupta wrote: > On Fri, Jul 6, 2018 at 10:55 PM Thomas Guillem wrote: > > > > > > > On Thu, Jul 5, 2018, at 20:49, Aman Gupta wrote: > > > On Wed, Jul 4, 2018 at 3:05 AM 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. > > > > --- > > > > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
On Fri, Jul 6, 2018 at 10:55 PM Thomas Guillem wrote: > > > On Thu, Jul 5, 2018, at 20:49, Aman Gupta wrote: > > On Wed, Jul 4, 2018 at 3:05 AM 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. > > > --- > > > 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 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
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 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
On Thu, Jul 5, 2018, at 20:49, Aman Gupta wrote: > On Wed, Jul 4, 2018 at 3:05 AM 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. > > --- > > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
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 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
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
On Wed, Jul 4, 2018 at 3:05 AM 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. > --- > 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 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
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] [PATCH 1/2] avcodec/videotoolboxenc: fix undefined behavior with rc_max_rate=0
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) { 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