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

[Michael Niedermayer]

On Thu, Sep 06, 2018 at 08:04:02PM -0300, James Almer wrote:
> On 9/6/2018 7:26 PM, Michael Niedermayer wrote:
> > On Thu, Sep 06, 2018 at 01:10:31PM -0300, James Almer wrote:
> >> 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? 
> > 
> > something like (with rounding up)
> > bytes * horizontal_chroma_subsampling / width * vertical_chroma_subsampling
> 
> Isn't a calculation like this already being done?

not sure i understand what you refer to


> 
> > 
> > 
> >> 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?
> > 
> > possible
> > 
> > 
> >>
> >> It would also be faster if you could commit that change instead.
> > 
> > thinking of this, its maybe simpler to adjust data[*] by these to get
> > exactly teh same effect as before
> 
> Is this before or after the merge? Because after the merge it's
> av_image_fill_pointers() who does all the work, and get_video_buffer()
> has no control over the pointers.

i meant after but maybe i miss something why the caller couldnt adjust the
pointers


> 
> Nothing about this is obvious to me, so i ask again if you could
> implement this instead. Otherwise I'll just no-op the merge and add it
> to the list of skipped changes in case someone else wants to give it a
> try at some other time.

ill take a look tomorrow, ping me in case i forget


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180910/c9889c5f/attachment.sig>