Discussion:
[PATCH] qsv: enforcing continuous memory layout
(too old to reply)
maxim_d33
2018-07-28 08:53:54 UTC
Permalink
---
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)
--
2.14.2
Luca Barbato
2018-07-28 09:54:58 UTC
Permalink
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
2018-07-30 05:39:28 UTC
Permalink
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
2018-07-30 11:19:02 UTC
Permalink
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
2018-07-30 11:21:25 UTC
Permalink
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
2018-07-28 10:20:43 UTC
Permalink
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
2018-07-28 22:50:42 UTC
Permalink
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
2018-07-29 17:38:13 UTC
Permalink
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
2018-07-30 16:04:13 UTC
Permalink
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

Loading...