[FFmpeg-devel] [PATCH] avcodec/snow: ensure current_picture is writable before modifying its data

[James Almer]

On 5/29/2020 7:38 PM, Michael Niedermayer wrote:
> On Fri, May 29, 2020 at 07:00:12PM -0300, James Almer wrote:
>> On 5/29/2020 5:37 PM, Michael Niedermayer wrote:
>>> On Fri, May 29, 2020 at 02:20:37PM -0300, James Almer wrote:
>>>> current_picture was not writable here because a reference existed in
>>>> at least avctx->coded_frame, and potentially elsewhere if the caller
>>>> created new ones from it.
>>>
>>> Please elaborate when and how the encoder internal buffer becomes
>>> read only
>>> i simply took your code and replaced
>>>
>>> -        ret = av_frame_make_writable(s->current_picture);
>>> -        if (ret < 0)
>>> -            return ret;
>>> +        ret = av_frame_is_writable(s->current_picture);
>>> +        if (ret <= 0)
>>> +            return -1;
>>>
>>> and fate is fully happy which tests both the encoder and the filters
>>> using it
>>> also if this is for coded_frame then shouldnt it be under FF_API_CODED_FRAME?
>>> iam clearly missing something here
>>
>> You need to also move the av_frame_unref(avctx->coded_frame) line back to
>> where it was before my patch. I moved it before this check to ensure at
>> least the reference stored there is freed before current_picture is written
>> to, 
> 
> i quite intentionally did not move that, my question was just about
> "why av_frame_make_writable" after all the other changes
> 
> 
>> but there of course could still exist other references created by the
>> user, and that's what this make_writable() call is for.
> 
> ok, understand this. I guess my gut feeling was that creating references
> to coded_frame was not allowed. but i guess there is nothing that forbids
> it so teh API allowes it, and the av_frame_make_writable is ok
>  
>  
>>
>> And since this specific chunk is not strictly coded_frame related, it
>> doesn't need to be under that guard.
> 
> but coded_frame is the only current way by which a user can create a
> reference, unless iam missing something
> Am i guessing correctly that you intend to add another way to export
> the frame or is there something iam missing ?

No, i expected you'd change this and find a way to get this
functionality in lavfi, since it's your code and the only non standard
coded_frame usage that's blocking its removal. I mentioned you as much
last major bump when we postponed the removal of coded_frame.
As i mentioned before, an encoder should not work as some sort of
interface to access lavc image processing features. Lavfi should either
use some lavc API, or feature its own implementation of this
functionality that's currently done here.

This patch is only to remove the current wrong behavior or writing on
potentially non writable frames, for the purpose of backporting to
existing branches (and so it's also present in 4.3). It should not
matter after the major bump.

> because without some other method this make_writable doesnt make sense
> without coded_frame and should be removed in case coded_frame is removed

Alright, i'll wrap it with the FF_API_ check before pushing.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>