diff mbox series

[FFmpeg-devel,2/5] tests/api-flac-test: ensure the frame is writable before writing to it

Message ID 20210224100402.22300-2-anton@khirnov.net
State Accepted
Commit ba30fd6c81261c0896e5755a4e1440c2792e2e89
Headers show
Series [FFmpeg-devel,1/5] tests/api-band-test: simplify code
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Anton Khirnov Feb. 24, 2021, 10:03 a.m. UTC
The encoder may keep a reference to frames that were sent to it, so the
caller cannot modify them without checking first.
---
 tests/api/api-flac-test.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

James Almer Feb. 24, 2021, 2:01 p.m. UTC | #1
On 2/24/2021 7:03 AM, Anton Khirnov wrote:
> The encoder may keep a reference to frames that were sent to it, so the
> caller cannot modify them without checking first.
> ---
>   tests/api/api-flac-test.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tests/api/api-flac-test.c b/tests/api/api-flac-test.c
> index 3fea3258f3..7c96a4d99e 100644
> --- a/tests/api/api-flac-test.c
> +++ b/tests/api/api-flac-test.c
> @@ -154,6 +154,10 @@ static int run_test(AVCodec *enc, AVCodec *dec, AVCodecContext *enc_ctx,
>           enc_pkt.data = NULL;
>           enc_pkt.size = 0;
>   
> +        result = av_frame_make_writable(in_frame);

This is going to make a copy of the existing frame data, only for the 
code below to completely replace it. It will also make copies (not 
references) of side data, if any.
Maybe instead unref it, set nb_samples, format and channel_layout again, 
then call av_frame_get_buffer() (Factoring the existing relevant code 
into its own function).

LGTM either way.

> +        if (result < 0)
> +            return result;
> +
>           generate_raw_frame((uint16_t*)(in_frame->data[0]), i, enc_ctx->sample_rate,
>                              enc_ctx->channels, enc_ctx->frame_size);
>           in_frame_bytes = in_frame->nb_samples * in_frame->channels * sizeof(uint16_t);
>
Anton Khirnov March 1, 2021, 1:43 p.m. UTC | #2
Quoting James Almer (2021-02-24 15:01:03)
> On 2/24/2021 7:03 AM, Anton Khirnov wrote:
> > The encoder may keep a reference to frames that were sent to it, so the
> > caller cannot modify them without checking first.
> > ---
> >   tests/api/api-flac-test.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/tests/api/api-flac-test.c b/tests/api/api-flac-test.c
> > index 3fea3258f3..7c96a4d99e 100644
> > --- a/tests/api/api-flac-test.c
> > +++ b/tests/api/api-flac-test.c
> > @@ -154,6 +154,10 @@ static int run_test(AVCodec *enc, AVCodec *dec, AVCodecContext *enc_ctx,
> >           enc_pkt.data = NULL;
> >           enc_pkt.size = 0;
> >   
> > +        result = av_frame_make_writable(in_frame);
> 
> This is going to make a copy of the existing frame data, only for the 
> code below to completely replace it. It will also make copies (not 
> references) of side data, if any.
> Maybe instead unref it, set nb_samples, format and channel_layout again, 
> then call av_frame_get_buffer() (Factoring the existing relevant code 
> into its own function).
> 
> LGTM either way.

Watches pelcome. I'd rather not spend more effort on this code than
strictly necessary.
James Almer March 1, 2021, 1:46 p.m. UTC | #3
On 3/1/2021 10:43 AM, Anton Khirnov wrote:
> Quoting James Almer (2021-02-24 15:01:03)
>> On 2/24/2021 7:03 AM, Anton Khirnov wrote:
>>> The encoder may keep a reference to frames that were sent to it, so the
>>> caller cannot modify them without checking first.
>>> ---
>>>    tests/api/api-flac-test.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/api/api-flac-test.c b/tests/api/api-flac-test.c
>>> index 3fea3258f3..7c96a4d99e 100644
>>> --- a/tests/api/api-flac-test.c
>>> +++ b/tests/api/api-flac-test.c
>>> @@ -154,6 +154,10 @@ static int run_test(AVCodec *enc, AVCodec *dec, AVCodecContext *enc_ctx,
>>>            enc_pkt.data = NULL;
>>>            enc_pkt.size = 0;
>>>    
>>> +        result = av_frame_make_writable(in_frame);
>>
>> This is going to make a copy of the existing frame data, only for the
>> code below to completely replace it. It will also make copies (not
>> references) of side data, if any.
>> Maybe instead unref it, set nb_samples, format and channel_layout again,
>> then call av_frame_get_buffer() (Factoring the existing relevant code
>> into its own function).
>>
>> LGTM either way.
> 
> Watches pelcome. I'd rather not spend more effort on this code than
> strictly necessary.

Sure, apply this and i'll change it later.
diff mbox series

Patch

diff --git a/tests/api/api-flac-test.c b/tests/api/api-flac-test.c
index 3fea3258f3..7c96a4d99e 100644
--- a/tests/api/api-flac-test.c
+++ b/tests/api/api-flac-test.c
@@ -154,6 +154,10 @@  static int run_test(AVCodec *enc, AVCodec *dec, AVCodecContext *enc_ctx,
         enc_pkt.data = NULL;
         enc_pkt.size = 0;
 
+        result = av_frame_make_writable(in_frame);
+        if (result < 0)
+            return result;
+
         generate_raw_frame((uint16_t*)(in_frame->data[0]), i, enc_ctx->sample_rate,
                            enc_ctx->channels, enc_ctx->frame_size);
         in_frame_bytes = in_frame->nb_samples * in_frame->channels * sizeof(uint16_t);