[FFmpeg-devel] [PATCH] frame: Simplify the video allocation

[James Almer]

On 9/4/2018 5:09 PM, Michael Niedermayer wrote:
> On Mon, Sep 03, 2018 at 10:29:13AM -0300, James Almer wrote:
>> On 9/3/2018 5:17 AM, Michael Niedermayer wrote:
>>> On Sun, Sep 02, 2018 at 09:34:23PM -0300, James Almer wrote:
>>>> From: Luca Barbato <lu_zero at gentoo.org>
>>>>
>>>> Merged-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>> This is the next merge in the queue. It's a critical part of the AVFrame API,
>>>> so even if FATE passes I'd rather have others look at it and test in case
>>>> something breaks.
>>>>
>>>> The only difference compared to the libav commit is the "32 - 1" padding per
>>>> plane when allocating the buffer, which was only in our tree.
>>>
>>> why is the STRIDE_ALIGN (which is a thing in units of bytes along the
>>> horizontal axis) added to padded_height which is vertical axis ?
>>> This is not done prior to the change
>>
>> The only way to keep this padding we currently have in the tree applied
>> to the buffer allocation for each plane like it was before the change
>> (Except it'll now be one continuous buffer instead of one per plane) is
>> by passing it alongside the height parameter to
>> av_image_fill_pointers(). The result is essentially the same.
>>
>> Do you want me to change the name of the variable, or remove it and pass
>> 32 - 1 to both av_image_fill_pointers() calls directly? Removing the
>> padding will probably just make whatever overreads prompted its addition
>> to resurface.
>> Alternatively, i can just no-op this merge and move on.
> 
> allocating one plane instead of 3 is better obviously so i dont think this
> should be no-oped unless someone implements this differently
> 
> i dont think the padding can be removed saftely but i might be missing something
> also i do not remember this 100%
> 
> what i see and i may have misunderstood your reply but the code before places
> a few bytes between planes, the new code places a few lines, that is alot more
> space. Its not even the best that can be done with the current API. For example
> the number of extra lines would generally be 1 to provide sufficient padding
> at most reaslistic resolutions.
> 
> also there is the independant question on the API, do we want/need to make 
> adding padding between planes easier?>
> actually i think that if we change from 31 bytes to X lines padding then this
> should be a commit seperate of the 3->1 change. This would make bisect much
> more meaningfull and its rather trivial to split this.

Do you have a suggestion on how to choose how many lines of padding to
add? And how would it be done? Just passing (h + padding_lines) to
av_buffer_alloc() pre merge, and to av_image_fill_pointers() post merge?

It would also be faster if you could commit that change instead.

> 
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>