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
