[FFmpeg-devel] frame: Simplify the video allocation

Submitted by James Almer on Sept. 3, 2018, 12:34 a.m.

Details

Message ID 20180903003423.6656-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Sept. 3, 2018, 12:34 a.m.
From: Luca Barbato <lu_zero@gentoo.org>

Merged-by: James Almer <jamrial@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.

 libavutil/frame.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

Comments

Michael Niedermayer Sept. 3, 2018, 8:17 a.m.
On Sun, Sep 02, 2018 at 09:34:23PM -0300, James Almer wrote:
> From: Luca Barbato <lu_zero@gentoo.org>
> 
> Merged-by: James Almer <jamrial@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

Also if this changes how buffers are structured or sized it requires a more
detailed commit message than "frame: Simplify the video allocation"

thanks

[...]
Maxym Dmytrychenko Sept. 3, 2018, 9:03 a.m.
On Mon, Sep 3, 2018 at 10:17 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sun, Sep 02, 2018 at 09:34:23PM -0300, James Almer wrote:
> > From: Luca Barbato <lu_zero@gentoo.org>
> >
> > Merged-by: James Almer <jamrial@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
>
> Also if this changes how buffers are structured or sized it requires a more
> detailed commit message than "frame: Simplify the video allocation"
>
>
If can help:
Use of av_image_fill_pointers() helps to allocate planes continuously
instead of separate allocation for each plane,
which can end up in very different start locations of the allocated memory.

Continuous allocation can be better for performance and/or functional sides
of the operations,
example of Intel's HW - QSV,
which is assuming Y/UV planes to be continuously allocated.




> thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I have often repented speaking, but never of holding my tongue.
> -- Xenocrates
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



thank you - James, Michael.

regards
Maxym
James Almer Sept. 3, 2018, 1:29 p.m.
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@gentoo.org>
>>
>> Merged-by: James Almer <jamrial@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.

> 
> Also if this changes how buffers are structured or sized it requires a more
> detailed commit message than "frame: Simplify the video allocation"

Suggestion welcome, but it would be added to the merge commit message.

> 
> thanks
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Darnley Sept. 3, 2018, 2:56 p.m.
On 2018-09-03 15:29, James Almer wrote:
> pass 32 - 1 to both av_image_fill_pointers() calls directly?

Please do not add a magic number where nobody will find it.  Use one of
the 3 already existing methods for knowing the alignment necessary for
assembly.

If this is unrelated, my apologies.
James Almer Sept. 3, 2018, 3:33 p.m.
On 9/3/2018 11:56 AM, James Darnley wrote:
> On 2018-09-03 15:29, James Almer wrote:
>> pass 32 - 1 to both av_image_fill_pointers() calls directly?
> 
> Please do not add a magic number where nobody will find it.  Use one of
> the 3 already existing methods for knowing the alignment necessary for
> assembly.

That magic number is STRIDE_ALIGN, and it's mentioned as such before and
after the patch. Except for the -1, which i don't know where it came
from. Michael (who i think added it) probably knows.

> 
> If this is unrelated, my apologies.
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Sept. 4, 2018, 8:09 p.m.
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@gentoo.org>
> >>
> >> Merged-by: James Almer <jamrial@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.


[...]
James Almer Sept. 6, 2018, 4:10 p.m.
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@gentoo.org>
>>>>
>>>> Merged-by: James Almer <jamrial@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Sept. 6, 2018, 10:26 p.m.
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@gentoo.org>
> >>>>
> >>>> Merged-by: James Almer <jamrial@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


> 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


thx

[...]
James Almer Sept. 6, 2018, 11:04 p.m.
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@gentoo.org>
>>>>>>
>>>>>> Merged-by: James Almer <jamrial@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?

> 
> 
>> 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.

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.

> 
> 
> thx
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Sept. 10, 2018, 1:30 a.m.
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@gentoo.org>
> >>>>>>
> >>>>>> Merged-by: James Almer <jamrial@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


[...]
James Almer Sept. 11, 2018, 4:42 p.m.
On 9/3/2018 6:03 AM, Maxym Dmytrychenko wrote:
> On Mon, Sep 3, 2018 at 10:17 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
>> On Sun, Sep 02, 2018 at 09:34:23PM -0300, James Almer wrote:
>>> From: Luca Barbato <lu_zero@gentoo.org>
>>>
>>> Merged-by: James Almer <jamrial@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
>>
>> Also if this changes how buffers are structured or sized it requires a more
>> detailed commit message than "frame: Simplify the video allocation"
>>
>>
> If can help:
> Use of av_image_fill_pointers() helps to allocate planes continuously
> instead of separate allocation for each plane,
> which can end up in very different start locations of the allocated memory.
> 
> Continuous allocation can be better for performance and/or functional sides
> of the operations,
> example of Intel's HW - QSV,
> which is assuming Y/UV planes to be continuously allocated.

I just merged this commit and the next, "qsv: enforcing continuous
memory layout" of your authoring, where one line checks the distance
between frame->data[1] and frame->data[0] to ensure the buffer is
continuous. This check, with the padding in our av_frame_get_buffer()
implementation, will probably always fail, but i can't test it.

Can you look into it, if that's indeed the case?
Maxym Dmytrychenko Sept. 11, 2018, 7:31 p.m.
On Tue, Sep 11, 2018 at 6:43 PM James Almer <jamrial@gmail.com> wrote:

> On 9/3/2018 6:03 AM, Maxym Dmytrychenko wrote:
> > On Mon, Sep 3, 2018 at 10:17 AM Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> >> On Sun, Sep 02, 2018 at 09:34:23PM -0300, James Almer wrote:
> >>> From: Luca Barbato <lu_zero@gentoo.org>
> >>>
> >>> Merged-by: James Almer <jamrial@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
> >>
> >> Also if this changes how buffers are structured or sized it requires a
> more
> >> detailed commit message than "frame: Simplify the video allocation"
> >>
> >>
> > If can help:
> > Use of av_image_fill_pointers() helps to allocate planes continuously
> > instead of separate allocation for each plane,
> > which can end up in very different start locations of the allocated
> memory.
> >
> > Continuous allocation can be better for performance and/or functional
> sides
> > of the operations,
> > example of Intel's HW - QSV,
> > which is assuming Y/UV planes to be continuously allocated.
>
> I just merged this commit and the next, "qsv: enforcing continuous
> memory layout" of your authoring, where one line checks the distance
> between frame->data[1] and frame->data[0] to ensure the buffer is
> continuous. This check, with the padding in our av_frame_get_buffer()
> implementation, will probably always fail, but i can't test it.
>
> Can you look into it, if that's indeed the case?
>

just finished my test cases and it seems to be just fine,
fixes the original, non-deterministic problem.

distance between frame->data[1] and frame->data[0]  is not always
non-continuous(and causes the problem)
where now av_frame_get_buffer()  fixes such corner case.

thanks, James

regards
Max

Patch hide | download patch | download mbox

diff --git a/libavutil/frame.c b/libavutil/frame.c
index deb9b6f334..f072baa916 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -211,7 +211,7 @@  void av_frame_free(AVFrame **frame)
 static int get_video_buffer(AVFrame *frame, int align)
 {
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
-    int ret, i;
+    int ret, i, padded_height;
 
     if (!desc)
         return AVERROR(EINVAL);
@@ -236,24 +236,18 @@  static int get_video_buffer(AVFrame *frame, int align)
             frame->linesize[i] = FFALIGN(frame->linesize[i], align);
     }
 
-    for (i = 0; i < 4 && frame->linesize[i]; i++) {
-        int h = FFALIGN(frame->height, 32);
-        if (i == 1 || i == 2)
-            h = AV_CEIL_RSHIFT(h, desc->log2_chroma_h);
+    padded_height = FFALIGN(frame->height, 32) + 32 /* STRIDE_ALIGN */ - 1;
+    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
+                                      NULL, frame->linesize)) < 0)
+        return ret;
 
-        frame->buf[i] = av_buffer_alloc(frame->linesize[i] * h + 16 + 16/*STRIDE_ALIGN*/ - 1);
-        if (!frame->buf[i])
-            goto fail;
+    frame->buf[0] = av_buffer_alloc(ret);
+    if (!frame->buf[0])
+        goto fail;
 
-        frame->data[i] = frame->buf[i]->data;
-    }
-    if (desc->flags & AV_PIX_FMT_FLAG_PAL || desc->flags & FF_PSEUDOPAL) {
-        av_buffer_unref(&frame->buf[1]);
-        frame->buf[1] = av_buffer_alloc(AVPALETTE_SIZE);
-        if (!frame->buf[1])
-            goto fail;
-        frame->data[1] = frame->buf[1]->data;
-    }
+    if (av_image_fill_pointers(frame->data, frame->format, padded_height,
+                               frame->buf[0]->data, frame->linesize) < 0)
+        goto fail;
 
     frame->extended_data = frame->data;