Re: [libav-devel] [PATCH v2] qsv: enforcing continuous memory layout
On 30/07/2018 18:02, Maxym Dmytrychenko wrote:
> we need to make sure that memory allocation for Y/UV planes is continuous and
> re-used from a
> pool
> ---
I'm afraid this would break the already-proper-frame codepath.
I would simplify the default avcoded allocator and call it directly.
> libavcodec/qsvenc.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index e349a075f..c74b3ae31 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1014,7 +1014,6 @@ static void clear_unused_frames(QSVEncContext *q)
> QSVFrame *cur = q->work_frames;
> while (cur) {
> if (cur->used && !cur->surface.Data.Locked) {
> -av_frame_unref(cur->frame);
> cur->used = 0;
> }
> cur = cur->next;
> @@ -1082,16 +1081,23 @@ static int submit_frame(QSVEncContext *q, const
> AVFrame *frame,
> }
> } else {
> /* make a copy if the input is not padded as libmfx requires */
> -if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1))
> {
> +/* and to make allocation continious for data[0]/data[1] */
> + if ((frame->height & 31 || frame->linesize[0] & (q->width_align -
> 1)) ||
> +(frame->data[1] - frame->data[0] != frame->linesize[0] *
> FFALIGN(qf->frame->height, q->height_align))) {
> qf->frame->height = FFALIGN(frame->height, q->height_align);
> qf->frame->width = FFALIGN(frame->width, q->width_align);
>
> -ret = ff_get_buffer(q->avctx, qf->frame, AV_GET_BUFFER_FLAG_REF);
> -if (ret < 0)
> -return ret;
> +qf->frame->format = frame->format;
> +
> +if (!qf->frame->data[0]) {
> +ret = av_frame_get_buffer(qf->frame, q->width_align);
> +if (ret < 0)
> +return ret;
> +}
>
> qf->frame->height = frame->height;
> qf->frame->width = frame->width;
> +
> ret = av_frame_copy(qf->frame, frame);
> if (ret < 0) {
> av_frame_unref(qf->frame);
>
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] qsv: enforcing continuous memory layout
On Sun, Jul 29, 2018 at 7:38 PM Diego Biurrun wrote: > On Sun, Jul 29, 2018 at 12:50:42AM +0200, Maxym Dmytrychenko wrote: > > On Sat, Jul 28, 2018 at 12:20 PM Diego Biurrun wrote: > > > On Sat, Jul 28, 2018 at 10:53:54AM +0200, maxim_d33 wrote: > > > > --- > > > > libavcodec/qsvenc.c | 34 -- > > > > 1 file changed, 24 insertions(+), 10 deletions(-) > > > > > > Looks like your Git is not set up properly. > > > > > > > > what do you mean exactly, Diego? > > I was squashing it before sending - may be because of this. > > The Git author name looks strange: maxim_d33. Probably because your > Git user.name is not set and falls back on your username. > > Diego > ___ > libav-devel mailing list > libav-devel@libav.org > https://lists.libav.org/mailman/listinfo/libav-devel ok, should be fixed at v2 regards Max ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
[libav-devel] [PATCH v2] qsv: enforcing continuous memory layout
we need to make sure that memory allocation for Y/UV planes is continuous and
re-used from a
pool
---
libavcodec/qsvenc.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index e349a075f..c74b3ae31 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1014,7 +1014,6 @@ static void clear_unused_frames(QSVEncContext *q)
QSVFrame *cur = q->work_frames;
while (cur) {
if (cur->used && !cur->surface.Data.Locked) {
-av_frame_unref(cur->frame);
cur->used = 0;
}
cur = cur->next;
@@ -1082,16 +1081,23 @@ static int submit_frame(QSVEncContext *q, const AVFrame
*frame,
}
} else {
/* make a copy if the input is not padded as libmfx requires */
-if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) {
+/* and to make allocation continious for data[0]/data[1] */
+ if ((frame->height & 31 || frame->linesize[0] & (q->width_align - 1))
||
+(frame->data[1] - frame->data[0] != frame->linesize[0] *
FFALIGN(qf->frame->height, q->height_align))) {
qf->frame->height = FFALIGN(frame->height, q->height_align);
qf->frame->width = FFALIGN(frame->width, q->width_align);
-ret = ff_get_buffer(q->avctx, qf->frame, AV_GET_BUFFER_FLAG_REF);
-if (ret < 0)
-return ret;
+qf->frame->format = frame->format;
+
+if (!qf->frame->data[0]) {
+ret = av_frame_get_buffer(qf->frame, q->width_align);
+if (ret < 0)
+return ret;
+}
qf->frame->height = frame->height;
qf->frame->width = frame->width;
+
ret = av_frame_copy(qf->frame, frame);
if (ret < 0) {
av_frame_unref(qf->frame);
--
2.14.2
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] qsv: enforcing continuous memory layout
On 30/07/2018 07:39, Li, Zhong wrote: > Could this make sure the reallocated aligned_frame is continuous memory? I made so it is in [libav-devel] [PATCH] frame: Simplify the video allocation. lu ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] qsv: enforcing continuous memory layout
On 30/07/2018 07:39, Li, Zhong wrote: > If yes, I guess the best fix is not to making a copy (it will > introduce performance drop). Continuous layout is required by spec, > not only for qsv. Currently nothing prevents anybody to implement a frame allocator that does not respects that. Incidentally the avcodec default frame allocator might enjoy something along the lines of what I did for the generic frame allocator in avutil Probably would be a good idea to update it as well. lu ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH v2] qsv: enforcing continuous memory layout
On Mon, 2018-07-30 at 18:02 +0200, Maxym Dmytrychenko wrote:
> we need to make sure that memory allocation for Y/UV planes is
> continuous and re-used from a
> pool
Could you, please, be more explicit in the commit message otherwise we
slip the discussion since not everyone here in the mailing list may
understand the origin of the problem?
I suggest to cover the following topics:
1. What's the problem? Why you submitted this patch? I.e. there was
random failures during some operations (which: decoding, encoding,
transcoding?)
2. Provide specific command line(s) with the examples where the issue
can be observed.
3. Highlight the root cause: hardware requires continuous memory
allocation and can't work with planes being allocated separately.
> ---
> libavcodec/qsvenc.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index e349a075f..c74b3ae31 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1014,7 +1014,6 @@ static void clear_unused_frames(QSVEncContext
> *q)
> QSVFrame *cur = q->work_frames;
> while (cur) {
> if (cur->used && !cur->surface.Data.Locked) {
> -av_frame_unref(cur->frame);
Hm. This call looked logical: you don't need a frame and you dispose of
it. Thus, it should either return to the pool or be completely
disposed. Why you removed this call? and where frame is being disposed now?
> cur->used = 0;
> }
> cur = cur->next;
> @@ -1082,16 +1081,23 @@ static int submit_frame(QSVEncContext *q,
> const AVFrame *frame,
> }
> } else {
> /* make a copy if the input is not padded as libmfx requires
> */
> -if (frame->height & 31 || frame->linesize[0] & (q-
> >width_align - 1)) {
> +/* and to make allocation continious for data[0]/data[1] */
> + if ((frame->height & 31 || frame->linesize[0] & (q-
> >width_align - 1)) ||
> +(frame->data[1] - frame->data[0] != frame->linesize[0] *
> FFALIGN(qf->frame->height, q->height_align))) {
So, if frame is allocated in an incompatible way, you trigger a copy,
right?
To be honest I consider this as a partial solution only. It is good
when you want to be compatible with the bigger range of 3rd party
components and for any reason you can't make sure that memory is
allocated in a compatible way right away...
Could we, please, discuss whether it is possible to have compatible
memory allocation from the very beginning? Question to libav guys: does
libav account that hardware usually requires continuous memory
allocations of the video memory? I.e. is there an libav API to handle
this?
Essentially to handle such a case negotiation stage between components
is needed. QSV in that case would inform other components that it
accepts only memory allocated in this particular way or that this is a
preferred option meaning that compatibility with non-continuous memory
will come with a price.
> qf->frame->height = FFALIGN(frame->height, q-
> >height_align);
> qf->frame->width = FFALIGN(frame->width, q-
> >width_align);
>
> -ret = ff_get_buffer(q->avctx, qf->frame,
> AV_GET_BUFFER_FLAG_REF);
> -if (ret < 0)
> -return ret;
> +qf->frame->format = frame->format;
> +
> +if (!qf->frame->data[0]) {
> +ret = av_frame_get_buffer(qf->frame, q-
> >width_align);
I was expecting to see some flag like AV_MEMORY_CONTINOUS to make sure
you get really continuous memory or a call to custom memory allocation
function which will make sure of it. The above call looks to be general
libav call, thus, generally speaking no guarantee memory will be
continuous.
> +if (ret < 0)
> +return ret;
> +}
>
> qf->frame->height = frame->height;
> qf->frame->width = frame->width;
> +
> ret = av_frame_copy(qf->frame, frame);
> if (ret < 0) {
> av_frame_unref(qf->frame);
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
