Re: [libav-devel] [PATCH 2/2] vaapi_h265: Mark unused entries in RefPicList[01] as explicitly invalid

[Jun Zhao]

On 2017/12/18 4:05, Mark Thompson wrote:
> The iHD driver looks at entries beyond num_ref_idx_l[01]_active_minus1
> for unknown reasons.
> ---
> This still isn't enough to actually work for encoding H.265 with the iHD 
> driver on Skylake.  It can generate output with this rather than crashing, 
> but the output is still wrong (though it does resemble the input somewhat).
>
> It also seems to like inserting emulation prevention bytes everywhere in 
> slice headers after the first, so it breaks totally if AUDs are present.
I think the root cause for HECV enc can't work in SKL is iHD have a
limitation for
max_transform_hierarchy_depth_inter/max_transform_hierarchy_depth_intra,
you can apply the patch in
https://github.com/mypopydev/FFmpeg/commit/fb35b6167d16d1acb81d0c82e19293c7cf97a574，
then re-try h265 ENC with iHD in SKL. Tks.
>
>  libavcodec/vaapi_encode_h265.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c
> index a9853a3aa..813d4f944 100644
> --- a/libavcodec/vaapi_encode_h265.c
> +++ b/libavcodec/vaapi_encode_h265.c
> @@ -767,8 +767,6 @@ static int 
> vaapi_encode_h265_init_slice_params(AVCodecContext *avctx,
>  
>  .num_ref_idx_l0_active_minus1 = sh->num_ref_idx_l0_active_minus1,
>  .num_ref_idx_l1_active_minus1 = sh->num_ref_idx_l1_active_minus1,
> -.ref_pic_list0[0] = vpic->reference_frames[0],
> -.ref_pic_list1[0] = vpic->reference_frames[1],
>  
>  .luma_log2_weight_denom = sh->luma_log2_weight_denom,
>  .delta_chroma_log2_weight_denom = sh->delta_chroma_log2_weight_denom,
> @@ -802,6 +800,25 @@ static int 
> vaapi_encode_h265_init_slice_params(AVCodecContext *avctx,
>  },
>  };
>  
> +for (i = 0; i < FF_ARRAY_ELEMS(vslice->ref_pic_list0); i++) {
> +vslice->ref_pic_list0[i].picture_id = VA_INVALID_ID;
> +vslice->ref_pic_list0[i].flags  = VA_PICTURE_HEVC_INVALID;
> +vslice->ref_pic_list1[i].picture_id = VA_INVALID_ID;
> +vslice->ref_pic_list1[i].flags  = VA_PICTURE_HEVC_INVALID;
> +}
> +
> +av_assert0(pic->nb_refs <= 2);
> +if (pic->nb_refs >= 1) {
> +// Backward reference for P- or B-frame.
> +av_assert0(pic->type == PICTURE_TYPE_P ||
> +   pic->type == PICTURE_TYPE_B);
> +vslice->ref_pic_list0[0] = vpic->reference_frames[0];
> +}
> +if (pic->nb_refs >= 2) {
> +// Forward reference for B-frame.
> +av_assert0(pic->type == PICTURE_TYPE_B);
> +vslice->ref_pic_list1[0] = vpic->reference_frames[1];
> +}
>  
>  return 0;
>  }

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/2] vaapi_encode: Destroy output buffer pool before VA context

[Jun Zhao]

On 2017/12/18 4:00, Mark Thompson wrote:
> The buffers are created associated with the context, so they should be
> destroyed before the context is.  This is enforced by the iHD driver.
> ---
> (Causes a crash on close.)
>
>
>  libavcodec/vaapi_encode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 47795ba73..fdc3d2a95 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -1536,6 +1536,8 @@ av_cold int ff_vaapi_encode_close(AVCodecContext *avctx)
>  vaapi_encode_free(avctx, pic);
>  }
>  
> +av_buffer_pool_uninit(>output_buffer_pool);
> +
>  if (ctx->va_context != VA_INVALID_ID) {
>  vaDestroyContext(ctx->hwctx->display, ctx->va_context);
>  ctx->va_context = VA_INVALID_ID;
> @@ -1546,8 +1548,6 @@ av_cold int ff_vaapi_encode_close(AVCodecContext *avctx)
>  ctx->va_config = VA_INVALID_ID;
>  }
>  
> -av_buffer_pool_uninit(>output_buffer_pool);
> -
>  av_freep(>codec_sequence_params);
>  av_freep(>codec_picture_params);
>  
I think is make sense, but Libva(VA-API) maybe need to update the
comments, and I think iHD will decouple the context and vabuffer when
need to destory.

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH 2/2] vaapi_h265: Mark unused entries in RefPicList[01] as explicitly invalid

[Mark Thompson]

The iHD driver looks at entries beyond num_ref_idx_l[01]_active_minus1
for unknown reasons.
---
This still isn't enough to actually work for encoding H.265 with the iHD driver 
on Skylake.  It can generate output with this rather than crashing, but the 
output is still wrong (though it does resemble the input somewhat).

It also seems to like inserting emulation prevention bytes everywhere in slice 
headers after the first, so it breaks totally if AUDs are present.


 libavcodec/vaapi_encode_h265.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c
index a9853a3aa..813d4f944 100644
--- a/libavcodec/vaapi_encode_h265.c
+++ b/libavcodec/vaapi_encode_h265.c
@@ -767,8 +767,6 @@ static int 
vaapi_encode_h265_init_slice_params(AVCodecContext *avctx,
 
 .num_ref_idx_l0_active_minus1 = sh->num_ref_idx_l0_active_minus1,
 .num_ref_idx_l1_active_minus1 = sh->num_ref_idx_l1_active_minus1,
-.ref_pic_list0[0] = vpic->reference_frames[0],
-.ref_pic_list1[0] = vpic->reference_frames[1],
 
 .luma_log2_weight_denom = sh->luma_log2_weight_denom,
 .delta_chroma_log2_weight_denom = sh->delta_chroma_log2_weight_denom,
@@ -802,6 +800,25 @@ static int 
vaapi_encode_h265_init_slice_params(AVCodecContext *avctx,
 },
 };
 
+for (i = 0; i < FF_ARRAY_ELEMS(vslice->ref_pic_list0); i++) {
+vslice->ref_pic_list0[i].picture_id = VA_INVALID_ID;
+vslice->ref_pic_list0[i].flags  = VA_PICTURE_HEVC_INVALID;
+vslice->ref_pic_list1[i].picture_id = VA_INVALID_ID;
+vslice->ref_pic_list1[i].flags  = VA_PICTURE_HEVC_INVALID;
+}
+
+av_assert0(pic->nb_refs <= 2);
+if (pic->nb_refs >= 1) {
+// Backward reference for P- or B-frame.
+av_assert0(pic->type == PICTURE_TYPE_P ||
+   pic->type == PICTURE_TYPE_B);
+vslice->ref_pic_list0[0] = vpic->reference_frames[0];
+}
+if (pic->nb_refs >= 2) {
+// Forward reference for B-frame.
+av_assert0(pic->type == PICTURE_TYPE_B);
+vslice->ref_pic_list1[0] = vpic->reference_frames[1];
+}
 
 return 0;
 }
-- 
2.11.0
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH 1/2] vaapi_encode: Destroy output buffer pool before VA context

[Mark Thompson]

The buffers are created associated with the context, so they should be
destroyed before the context is.  This is enforced by the iHD driver.
---
(Causes a crash on close.)


 libavcodec/vaapi_encode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index 47795ba73..fdc3d2a95 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -1536,6 +1536,8 @@ av_cold int ff_vaapi_encode_close(AVCodecContext *avctx)
 vaapi_encode_free(avctx, pic);
 }
 
+av_buffer_pool_uninit(>output_buffer_pool);
+
 if (ctx->va_context != VA_INVALID_ID) {
 vaDestroyContext(ctx->hwctx->display, ctx->va_context);
 ctx->va_context = VA_INVALID_ID;
@@ -1546,8 +1548,6 @@ av_cold int ff_vaapi_encode_close(AVCodecContext *avctx)
 ctx->va_config = VA_INVALID_ID;
 }
 
-av_buffer_pool_uninit(>output_buffer_pool);
-
 av_freep(>codec_sequence_params);
 av_freep(>codec_picture_params);
 
-- 
2.11.0
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [RFC] Getting rid of the global log callback

[Luca Barbato]

On 17/12/2017 15:17, Rémi Denis-Courmont wrote:

user_context may need a user_free(),


Why oh why?


For consistency and to avoid some ugly corner cases.


There should be a point when the libavcodec user application can safely assume
that the callback will no longer be used - e.g. when the context is closed.
Using a free callback is only making things needlessly brittle and complicated
on both sides.


I totally agree, but I'm quite afraid this totally reasonable and valid 
assumption might or might not be always valid, thus the may in the comments.


If there is agreement on the use case I'm more than happy to have less 
fields to fill.


lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [RFC] Getting rid of the global log callback

[wm4]

On Sun, 17 Dec 2017 16:17:47 +0200
Rémi Denis-Courmont  wrote:

> Le sunnuntaina 17. joulukuuta 2017, 14.35.13 EET Luca Barbato a écrit :
> > On 11/12/2017 14:48, wm4 wrote:  
> > > The log callback, set with av_log_set_callback(), is global mutable
> > > state, and as such not something we want in Libav, at all. but getting
> > > rid of it is very complicated, because in most cases, av_log() does not
> > > have enough context available to find per-context log callbacks.
> > > 
> > > av_log() has a context pointer as first argument. This is just a void*.
> > > If it's non-NULL, then it's assumed to be a struct, with a AVClass*
> > > field as first member (e.g. AVCodecContext.av_class). This points to
> > > const memory, so it's not mutable state, and there's no way to put a
> > > private log callback there.
> > > 
> > > So this whole problem boils down to how to change AVClass to store a
> > > per-context log callback in structs using AVClass.
> > > 
> > > You can pass a NULL context to av_log() too. This would inherently
> > > require a global log callback, so we have to deprecate this usage
> > > entirely.
> > > 
> > > A log callback would include a pointer, and a user pointer:
> > > 
> > > struct AVLogCallback {
> > > 
> > >void *user_context;
> > >// context is the caller (a struct with AVClass* as first field)
> > >// user_context as the same value as the field above.
> > >void (*callback)(void *context, void *user_context, int level,
> > >
> > > const char *fmt, va_list vl);
> > > 
> > > }  
> > 
> > user_context may need a user_free(),  
> 
> Why oh why?
> 
> There should be a point when the libavcodec user application can safely 
> assume 
> that the callback will no longer be used - e.g. when the context is closed. 
> Using a free callback is only making things needlessly brittle and 
> complicated 
> on both sides.
> 

Well, he has a point. Some things live on as long something keeps a
reference to it. This can be quite non-obvious (like an AVFrame
implicitly holding a decoder frame pool and such, after the decoder
was closed).

So maybe the callback should be refcounted, if only to give the API
user the chance to safely free whatever resource the callback needs.

But it does feel a bit like getting into some bikeshed downwards spiral.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [RFC] Getting rid of the global log callback

[Luca Barbato]

On 11/12/2017 14:48, wm4 wrote:

The log callback, set with av_log_set_callback(), is global mutable
state, and as such not something we want in Libav, at all. but getting
rid of it is very complicated, because in most cases, av_log() does not
have enough context available to find per-context log callbacks.

av_log() has a context pointer as first argument. This is just a void*.
If it's non-NULL, then it's assumed to be a struct, with a AVClass*
field as first member (e.g. AVCodecContext.av_class). This points to
const memory, so it's not mutable state, and there's no way to put a
private log callback there.

So this whole problem boils down to how to change AVClass to store a
per-context log callback in structs using AVClass.

You can pass a NULL context to av_log() too. This would inherently
require a global log callback, so we have to deprecate this usage
entirely.

A log callback would include a pointer, and a user pointer:

struct AVLogCallback {
   void *user_context;
   // context is the caller (a struct with AVClass* as first field)
   // user_context as the same value as the field above.
   void (*callback)(void *context, void *user_context, int level,
const char *fmt, va_list vl);
}


user_context may need a user_free(),


Personally, I'd prefer if we had mandatory state per struct. Let's call
it AVObject:

struct AVObject {
   const AVClass *av_class;
   AVLogCallback log_callback:
}

(Sorry for the bad taste in names, but this whole AVClass nonsense
wasn't my idea anyway.)

We'd have a new AVClass field to signal presence of this:

struct AVClass {
   const char* class_name;
   ...
   int is_new_class;
}

The following function would work on my Libav context that works with
av_log() (provided all conversions have been done):

int av_class_set_log_callback(void *ctx, AVLogCallback cb);

// Example usage:
struct AVCodecContext {
   AVObject object;
   // ... other fields
}

AVCodecContext *avctx = ...;
av_class_set_log_callback(avctx, cb);


This probably won't work, because:

1. AVCodecContext.av_class probably can't go away
2. Would require an ABI bump too, which might be too late now, or not
good for incremental conversion

So instead of AVObject/is_new_class we'd just have another field
offset, which I think is a bit clunky and possibly braindamaged, but
it would work. We would have:


If it is fine to copy it around you can simply alloc+copy and store the 
pointer I guess.


I doubt we'll have to do that in a hotpath anyway.


I'm arguing for such an extensible AVClassState, so we can stuff other
things like AVClass.log_level_offset_offset in there. Possibly settings
like CPU flags can go there too.


Might not hurt. As long it is something that you use at setup time it is 
fine to fit it there.



I'm also against something like a AVLibraryState struct, which would
imply to be globally shared across all contexts. I think it's better if
the state gets "inherited" by copying all the parameters as contexts
create sub-contexts. This is simpler and more flexible, and instead of
having the user pass around such an AVLibraryState, the user would just
configure the parameters with calls like av_class_set_log_callback() on
contexts.


We can make that an implementation detail by passing the opaque to the 
_alloc() function. It is either copied over or ref-counted-shared.


If we copy the struct around we should add a user_copy() next to the 
user_free() callback thought.



Anyway, once these mechanisms are in place, we'd deprecate the global
log callback, and the possibility to pass NULL as first argument to
av_log().

Thoughts? I'd like to implement this stuff once we agree on how to
proceed.

Beside the remarks above I'm fine with this plan.

lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel