[FFmpeg-devel] [PATCH] avutil/avframe: fix channel layout checks in av_frame_copy()

[FFmpeg-devel] [PATCH] avutil/avframe: fix channel layout checks in av_frame_copy()

[James Almer]

Normally, both the source and dest frame would have only the old API fields
set, only the new API fields set, or both set. But in some cases, like when
calling av_frame_ref() using a non reference counted source frame where only
the old channel layout API fields were populated, the result would be the dst
frame having both the new and old fields populated.

This commit takes this into account and fixes the checks by calling
av_channel_layout_compare() only if the source frame has the new API fields
set, and doing sanity checks for the source frame old API fields if the new
ones are not set.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index fbb869fffa..0da4bb4768 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -727,19 +727,30 @@ static int frame_copy_audio(AVFrame *dst, const AVFrame *src)
 
 #if FF_API_OLD_CHANNEL_LAYOUT
 FF_DISABLE_DEPRECATION_WARNINGS
+    int channel_layout = dst->ch_layout.order == AV_CHANNEL_ORDER_NATIVE ?
+                         dst->ch_layout.u.mask : 0;
     if (!channels) {
-        if (dst->channels       != src->channels ||
-            dst->channel_layout != src->channel_layout)
-            return AVERROR(EINVAL);
         channels = dst->channels;
+        channel_layout = dst->channel_layout;
         planes = planar ? channels : 1;
+    }
+    if (!src->ch_layout.nb_channels) {
+        if (channels       != src->channels ||
+            channel_layout != src->channel_layout)
+            return AVERROR(EINVAL);
         CHECK_CHANNELS_CONSISTENCY(src);
     }
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
 
     if (dst->nb_samples != src->nb_samples ||
+#if FF_API_OLD_CHANNEL_LAYOUT
+        (av_channel_layout_check(&src->ch_layout) &&
+#endif
         av_channel_layout_compare(&dst->ch_layout, &src->ch_layout))
+#if FF_API_OLD_CHANNEL_LAYOUT
+        )
+#endif
         return AVERROR(EINVAL);
 
     for (i = 0; i < planes; i++)
@@ -763,7 +774,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
     else if (dst->nb_samples > 0 &&
              (av_channel_layout_check(&dst->ch_layout)
 #if FF_API_OLD_CHANNEL_LAYOUT
-              || dst->channel_layout || dst->channels
+              || dst->channels > 0
 #endif
             ))
         return frame_copy_audio(dst, src);
-- 
2.36.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/avframe: fix channel layout checks in av_frame_copy()

[Tomas Härdin]

ons 2022-05-25 klockan 14:15 -0300 skrev James Almer:
> Normally, both the source and dest frame would have only the old API
> fields
> set, only the new API fields set, or both set. But in some cases,
> like when
> calling av_frame_ref() using a non reference counted source frame
> where only
> the old channel layout API fields were populated, the result would be
> the dst
> frame having both the new and old fields populated.
> 
> This commit takes this into account and fixes the checks by calling
> av_channel_layout_compare() only if the source frame has the new API
> fields
> set, and doing sanity checks for the source frame old API fields if
> the new
> ones are not set.

A bit of context for those wondering: this is part of ensuring melt
works both with FFmpeg 5.0 and the new channel layout API. They're
still using deprecated things, but one step at a time.

> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavutil/frame.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

Works

/Tomas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/avframe: fix channel layout checks in av_frame_copy()

[James Almer]

On 5/27/2022 5:12 AM, Tomas Härdin wrote:
> ons 2022-05-25 klockan 14:15 -0300 skrev James Almer:
>> Normally, both the source and dest frame would have only the old API
>> fields
>> set, only the new API fields set, or both set. But in some cases,
>> like when
>> calling av_frame_ref() using a non reference counted source frame
>> where only
>> the old channel layout API fields were populated, the result would be
>> the dst
>> frame having both the new and old fields populated.
>>
>> This commit takes this into account and fixes the checks by calling
>> av_channel_layout_compare() only if the source frame has the new API
>> fields
>> set, and doing sanity checks for the source frame old API fields if
>> the new
>> ones are not set.
> 
> A bit of context for those wondering: this is part of ensuring melt
> works both with FFmpeg 5.0 and the new channel layout API. They're
> still using deprecated things, but one step at a time.

I assume it may be affecting other library users, since it's a matter of 
passing an AVFrame with no reference counted buffers (nothing in buf[], 
only data[] pointers filled) and only the old channel layout API fields 
set to av_frame_ref() or anything that calls it, like avcodec_send_frame().

The amount of users of the new channel layout at this point is probably 
very small, since it's not present in any release. I can't say how many 
users passing AVFrames with non reference counted buffers are out there, 
but it's a supported scenario, so melt may not be the only case.

> 
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavutil/frame.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> Works
> 
> /Tomas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/avframe: fix channel layout checks in av_frame_copy()

[James Almer]

On 5/25/2022 2:15 PM, James Almer wrote:
> Normally, both the source and dest frame would have only the old API fields
> set, only the new API fields set, or both set. But in some cases, like when
> calling av_frame_ref() using a non reference counted source frame where only
> the old channel layout API fields were populated, the result would be the dst
> frame having both the new and old fields populated.
> 
> This commit takes this into account and fixes the checks by calling
> av_channel_layout_compare() only if the source frame has the new API fields
> set, and doing sanity checks for the source frame old API fields if the new
> ones are not set.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>   libavutil/frame.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)

Will apply.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".