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

[maxim_d33]

---
libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index e349a075f..9fb1ae01b 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1061,6 +1061,7 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame,
{
QSVFrame *qf;
int ret;
+ AVFrame* aligned_frame;

ret = get_free_frame(q, &qf);
if (ret < 0)
@@ -1081,22 +1082,35 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame,
qf->surface.Data.MemId = &q->frames_ctx.mids[ret];
}
} else {
- /* make a copy if the input is not padded as libmfx requires */
- if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) {
- 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);
+ /* make a copy if
+ - the input is not padded
+ - allocations are not continious for data[0]/data[1]
+ required by libmfx */
+ if ((frame->data[1] - frame->data[0] != frame->linesize[0] * FFALIGN(frame->height, q->height_align)) ||
+ (frame->height & 31 || frame->linesize[0] & (q->width_align - 1))) {
+ aligned_frame = av_frame_alloc();
+ if (!aligned_frame)
+ return AVERROR(ENOMEM);
+
+ aligned_frame->format = frame->format;
+ aligned_frame->height = FFALIGN(frame->height, q->height_align);
+ aligned_frame->width = FFALIGN(frame->width, q->width_align);
+
+ ret = av_frame_get_buffer(aligned_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);
+ aligned_frame->height = frame->height;
+ aligned_frame->width = frame->width;
+
+ ret = av_frame_copy(aligned_frame, frame);
if (ret < 0) {
- av_frame_unref(qf->frame);
+ av_frame_unref(aligned_frame);
return ret;
}
+
+ av_frame_free(&qf->frame);
+ qf->frame = aligned_frame;
} else {
ret = av_frame_ref(qf->frame, frame);
if (ret < 0)

[Luca Barbato]

Post by maxim_d33
---
libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index e349a075f..9fb1ae01b 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1061,6 +1061,7 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame,
{
QSVFrame *qf;
int ret;
+ AVFrame* aligned_frame;
ret = get_free_frame(q, &qf);
if (ret < 0)
@@ -1081,22 +1082,35 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame,
qf->surface.Data.MemId = &q->frames_ctx.mids[ret];
}
} else {
- /* make a copy if the input is not padded as libmfx requires */
- if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) {

This can be kept

Post by maxim_d33
- qf->frame->height = FFALIGN(frame->height, q->height_align);
- qf->frame->width = FFALIGN(frame->width, q->width_align);

I think

Post by maxim_d33
- ret = ff_get_buffer(q->avctx, qf->frame, AV_GET_BUFFER_FLAG_REF);
+ /* make a copy if
+ - the input is not padded
+ - allocations are not continious for data[0]/data[1]
+ required by libmfx */
+ if ((frame->data[1] - frame->data[0] != frame->linesize[0] * FFALIGN(frame->height, q->height_align)) ||
+ (frame->height & 31 || frame->linesize[0] & (q->width_align - 1))) {
+ aligned_frame = av_frame_alloc();
+ if (!aligned_frame)
+ return AVERROR(ENOMEM);

I think you can use qf->frame instead of creating another all the time.

Post by maxim_d33
+ aligned_frame->format = frame->format;
+ aligned_frame->height = FFALIGN(frame->height, q->height_align);
+ aligned_frame->width = FFALIGN(frame->width, q->width_align);
+
+ ret = av_frame_get_buffer(aligned_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);
+ aligned_frame->height = frame->height;
+ aligned_frame->width = frame->width;
+
+ ret = av_frame_copy(aligned_frame, frame);
if (ret < 0) {
- av_frame_unref(qf->frame);
+ av_frame_unref(aligned_frame);
return ret;
}
+
+ av_frame_free(&qf->frame);
+ qf->frame = aligned_frame;
} else {
ret = av_frame_ref(qf->frame, frame);
if (ret < 0)

lu

[Li, Zhong]

Barbato
Sent: Saturday, July 28, 2018 5:55 PM
Subject: Re: [libav-devel] [PATCH] qsv: enforcing continuous memory layout

Post by maxim_d33
---
libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index
e349a075f..9fb1ae01b 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1061,6 +1061,7 @@ static int submit_frame(QSVEncContext *q,

const

Post by maxim_d33
AVFrame *frame, {
QSVFrame *qf;
int ret;
+ AVFrame* aligned_frame;
ret = get_free_frame(q, &qf);
if (ret < 0)
@@ -1081,22 +1082,35 @@ static int submit_frame(QSVEncContext *q,

const AVFrame *frame,

Post by maxim_d33
qf->surface.Data.MemId = &q->frames_ctx.mids[ret];
}
} else {
- /* make a copy if the input is not padded as libmfx requires */
- if (frame->height & 31 || frame->linesize[0] & (q->width_align -

1)) {
This can be kept

Post by maxim_d33
- qf->frame->height = FFALIGN(frame->height,

q->height_align);

Post by maxim_d33
- qf->frame->width = FFALIGN(frame->width,

q->width_align);
I think

Post by maxim_d33
- ret = ff_get_buffer(q->avctx, qf->frame,

AV_GET_BUFFER_FLAG_REF);

Post by maxim_d33
+ /* make a copy if
+ - the input is not padded
+ - allocations are not continious for data[0]/data[1]
+ required by libmfx */
+ if ((frame->data[1] - frame->data[0] != frame->linesize[0] *

FFALIGN(frame->height, q->height_align)) ||

Thanks a lot for this patch, should be useful for http://trac.ffmpeg.org/ticket/5708.
IIRC, this is only required for NV12 format. For YUV420p, is it must too?
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.

Checked https://www.fourcc.org/pixel-format/yuv-nv12/:
"A format in which all Y samples are found first in memory as an array of unsigned char with an even number of lines (possibly with a larger stride for memory alignment), _followed immediately_ by an array of unsigned char containing interleaved Cb and Cr samples (such that if addressed as a little-endian WORD type, Cb would be in the LSBs and Cr would be in the MSBs) with the same total stride as the Y samples. This is the preferred 4:2:0 pixel format."

So I think the best fixing it in the initial allocation.

Post by maxim_d33
+ (frame->height & 31 || frame->linesize[0] &

(q->width_align - 1))) {

Post by maxim_d33
+ aligned_frame = av_frame_alloc();
+ if (!aligned_frame)
+ return AVERROR(ENOMEM);

I think you can use qf->frame instead of creating another all the time.

Post by maxim_d33
+ aligned_frame->format = frame->format;
+ aligned_frame->height = FFALIGN(frame->height,

q->height_align);

Post by maxim_d33
+ aligned_frame->width = FFALIGN(frame->width,
+ q->width_align);
+
+ ret = av_frame_get_buffer(aligned_frame,q->width_align);

Could this make sure the reallocated aligned_frame is continuous memory?

Post by maxim_d33
if (ret < 0)
return ret;
- qf->frame->height = frame->height;
- qf->frame->width = frame->width;
- ret = av_frame_copy(qf->frame, frame);
+ aligned_frame->height = frame->height;
+ aligned_frame->width = frame->width;
+
+ ret = av_frame_copy(aligned_frame, frame);
if (ret < 0) {
- av_frame_unref(qf->frame);
+ av_frame_unref(aligned_frame);
return ret;
}
+
+ av_frame_free(&qf->frame);
+ qf->frame = aligned_frame;
} else {
ret = av_frame_ref(qf->frame, frame);
if (ret < 0)

lu

[Luca Barbato]

Post by Li, Zhong
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

[Luca Barbato]

Post by Li, Zhong
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

[Diego Biurrun]

Post by maxim_d33
---
libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)

Looks like your Git is not set up properly.

Diego

[Maxym Dmytrychenko]

Hi Diego!

Post by Diego Biurrun

Post by maxim_d33
---
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.

Post by Diego Biurrun
Diego
_______________________________________________
libav-devel mailing list
https://lists.libav.org/mailman/listinfo/libav-devel

regards
Max

[Diego Biurrun]

Post by Maxym Dmytrychenko

Post by Diego Biurrun

Post by maxim_d33
---
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

[Maxym Dmytrychenko]

Post by Diego Biurrun

Post by Maxym Dmytrychenko

Post by Diego Biurrun

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

ok, should be fixed at v2

regards
Max