[libav-devel] [PATCH v2] qsv: enforcing continuous memory layout

[Maxym Dmytrychenko]

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);

[Luca Barbato]

Post by Maxym Dmytrychenko
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.

Post by Maxym Dmytrychenko
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);

[Maxym Dmytrychenko]

Post by Maxym Dmytrychenko

Post by Maxym Dmytrychenko
we need to make sure that memory allocation for Y/UV planes is

continuous and re-used from a

Post by Maxym Dmytrychenko
pool
---

I'm afraid this would break the already-proper-frame codepath.
I would simplify the default avcoded allocator and call it directly.

Note that:

+ if (!qf->frame->data[0]) {
+ ret = av_frame_get_buffer(qf->frame, q->width_align);

would keep the pool running and dont see it leaking, or?

Post by Maxym Dmytrychenko

Post by Maxym Dmytrychenko
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,

Post by Maxym Dmytrychenko
}
} else {
/* make a copy if the input is not padded as libmfx requires */
- if (frame->height & 31 || frame->linesize[0] & (q->width_align

- 1)) {

Post by Maxym Dmytrychenko
+ /* and to make allocation continious for data[0]/data[1] */
+ if ((frame->height & 31 || frame->linesize[0] &

(q->width_align - 1)) ||

Post by Maxym Dmytrychenko
+ (frame->data[1] - frame->data[0] != frame->linesize[0] *

FFALIGN(qf->frame->height, q->height_align))) {

Post by Maxym Dmytrychenko
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);

Post by Maxym Dmytrychenko
- 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
https://lists.libav.org/mailman/listinfo/libav-devel

[Luca Barbato]

Post by Maxym Dmytrychenko
+ if (!qf->frame->data[0]) {
+ ret = av_frame_get_buffer(qf->frame, q->width_align);
would keep the pool running and dont see it leaking, or?

You seem right, I guess the patch is in the good direction, reworking
the code to use the normal framepool might be a good idea though.

lu

[Rogozhkin, Dmitry V]

Post by Maxym Dmytrychenko
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.

Post by Maxym Dmytrychenko
---
 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?

Post by Maxym Dmytrychenko
             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-

Post by Maxym Dmytrychenko
width_align - 1)) {

+        /* and to make allocation continious for data[0]/data[1] */
+         if ((frame->height & 31 || frame->linesize[0] & (q-

Post by Maxym Dmytrychenko
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.

Post by Maxym Dmytrychenko
             qf->frame->height = FFALIGN(frame->height, q-

Post by Maxym Dmytrychenko
height_align);

             qf->frame->width  = FFALIGN(frame->width, q-

Post by Maxym Dmytrychenko
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-

Post by Maxym Dmytrychenko
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.

Post by Maxym Dmytrychenko
+                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);