[FFmpeg-devel,5/5] amfenc: Remove spurious initialisations

Submitted by Mark Thompson on April 14, 2018, 3:53 p.m.

Details

Message ID 20180414155339.22839-5-sw@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson April 14, 2018, 3:53 p.m.
Also minor cosmetics.
---
 libavcodec/amfenc.c | 70 ++++++++++++++++++++---------------------------------
 1 file changed, 26 insertions(+), 44 deletions(-)

Comments

Alexander Kravchenko April 14, 2018, 11:17 p.m.
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Saturday, April 14, 2018 6:54 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 5/5] amfenc: Remove spurious initialisations
> 

Hi Mark,
I reviewed and tested all 5 patches in set all together.
Now the code looks cleaner.
My local tests passed for dx9 and dx11 with and without -hwaccel_output_format.

I am waiting patches to be applied to propose new patch with hwcontext_amf in libavutil.

Thanks,
Alexander
Mark Thompson April 15, 2018, 4:30 p.m.
On 15/04/18 00:17, Alexander Kravchenko wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson
>> Sent: Saturday, April 14, 2018 6:54 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH 5/5] amfenc: Remove spurious initialisations
>>
> 
> Hi Mark,
> I reviewed and tested all 5 patches in set all together.
> Now the code looks cleaner.
> My local tests passed for dx9 and dx11 with and without -hwaccel_output_format.

Ok, applied.  Thank you!

> I am waiting patches to be applied to propose new patch with hwcontext_amf in libavutil.

Can you explain what you're intending to use that for?  It's not clear to me how an extra wrapper around the D3D(9|11) surfaces is going to help, given that the support for them with AMF is already pretty good.  (Compare the Intel libmfx stuff (the misleadingly-named "qsv") where the extra wrapping does help for some cases because the underlying library has weird constraints, but overall adds a lot of complexity (and failure modes) for rather unclear benefit.  It's also inconvenient in that it promotes the existence of antifeatures like the "_qsv" decoders which are inferior to the builtin hwaccels in pretty much every respect.)

- Mark
Alexander Kravchenko April 15, 2018, 7:45 p.m.
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Sunday, April 15, 2018 7:31 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 5/5] amfenc: Remove spurious initialisations
> 
> 
> > I am waiting patches to be applied to propose new patch with hwcontext_amf in libavutil.
> 
> Can you explain what you're intending to use that for?  It's not clear to me how an extra wrapper around the D3D(9|11) surfaces is
> going to help, given that the support for them with AMF is already pretty good.  (Compare the Intel libmfx stuff (the misleadingly-
> named "qsv") where the extra wrapping does help for some cases because the underlying library has weird constraints, but overall
> adds a lot of complexity (and failure modes) for rather unclear benefit.  It's also inconvenient in that it promotes the existence of
> antifeatures like the "_qsv" decoders which are inferior to the builtin hwaccels in pretty much every respect.)
> 

Hi Mark,
I am intending to create amf common helpers(tools) in libavutil. 
They will be used in amfenc and vf_scaleamf. (vf_scaleamf is hw-accelerated color-space converter and scaler)

amf helpers should provide:
* amf_library: functions to load/unload amf dll based on reference count mechanism
* amf_context: functions to create AMFContext derived from DXVA2, D3D11, opencl and Vulcan in future
* av_frame <-> AMFSurface map functions (move from amfenc.c)

amfav_context can be implemented like hwdevice_ctx (AVAMFDeviceContext) and can be managed by av_hwdevice_ctx_create_derived (in case of incoming hw frames) and av_hwdevice_ctx_create or it can be implemented not using of av_hwdevice_ctx* mechanism

I think don’t need AVAMFFrameContext, and amf components will send/receive hwframes based on existing framecontexts (dxva/opencl...)

Could you recommend the best way how to do this fit in current architecture?

Thanks,
Alexander

Patch hide | download patch | download mbox

diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
index e641048532..93fcee9480 100644
--- a/libavcodec/amfenc.c
+++ b/libavcodec/amfenc.c
@@ -107,16 +107,11 @@  static AMFTraceWriterVtbl tracer_vtbl =
 
 static int amf_load_library(AVCodecContext *avctx)
 {
-    AmfContext             *ctx = avctx->priv_data;
-    AMFInit_Fn              init_fun = NULL;
-    AMFQueryVersion_Fn      version_fun = NULL;
-    AMF_RESULT              res = AMF_OK;
+    AmfContext        *ctx = avctx->priv_data;
+    AMFInit_Fn         init_fun;
+    AMFQueryVersion_Fn version_fun;
+    AMF_RESULT         res;
 
-    ctx->eof = 0;
-    ctx->delayed_drain = 0;
-    ctx->hw_frames_ctx = NULL;
-    ctx->hw_device_ctx = NULL;
-    ctx->delayed_surface = NULL;
     ctx->delayed_frame = av_frame_alloc();
     if (!ctx->delayed_frame) {
         return AVERROR(ENOMEM);
@@ -326,10 +321,10 @@  static int amf_init_context(AVCodecContext *avctx)
 
 static int amf_init_encoder(AVCodecContext *avctx)
 {
-    AmfContext          *ctx = avctx->priv_data;
-    const wchar_t       *codec_id = NULL;
-    AMF_RESULT           res = AMF_OK;
-    enum AVPixelFormat   pix_fmt;
+    AmfContext        *ctx = avctx->priv_data;
+    const wchar_t     *codec_id = NULL;
+    AMF_RESULT         res;
+    enum AVPixelFormat pix_fmt;
 
     switch (avctx->codec->id) {
         case AV_CODEC_ID_H264:
@@ -360,9 +355,9 @@  static int amf_init_encoder(AVCodecContext *avctx)
 
 int av_cold ff_amf_encode_close(AVCodecContext *avctx)
 {
-    AmfContext      *ctx = avctx->priv_data;
-    if (ctx->delayed_surface)
-    {
+    AmfContext *ctx = avctx->priv_data;
+
+    if (ctx->delayed_surface) {
         ctx->delayed_surface->pVtbl->Release(ctx->delayed_surface);
         ctx->delayed_surface = NULL;
     }
@@ -402,11 +397,11 @@  int av_cold ff_amf_encode_close(AVCodecContext *avctx)
 static int amf_copy_surface(AVCodecContext *avctx, const AVFrame *frame,
     AMFSurface* surface)
 {
-    AMFPlane       *plane = NULL;
-    uint8_t        *dst_data[4];
-    int             dst_linesize[4];
-    int             planes;
-    int             i;
+    AMFPlane *plane;
+    uint8_t  *dst_data[4];
+    int       dst_linesize[4];
+    int       planes;
+    int       i;
 
     planes = surface->pVtbl->GetPlanesCount(surface);
     av_assert0(planes < FF_ARRAY_ELEMS(dst_data));
@@ -437,11 +432,11 @@  static inline int timestamp_queue_enqueue(AVCodecContext *avctx, int64_t timesta
 
 static int amf_copy_buffer(AVCodecContext *avctx, AVPacket *pkt, AMFBuffer *buffer)
 {
-    AmfContext             *ctx = avctx->priv_data;
-    int                     ret;
-    AMFVariantStruct        var = {0};
-    int64_t                 timestamp = AV_NOPTS_VALUE;
-    int64_t                 size = buffer->pVtbl->GetSize(buffer);
+    AmfContext      *ctx = avctx->priv_data;
+    int              ret;
+    AMFVariantStruct var = {0};
+    int64_t          timestamp = AV_NOPTS_VALUE;
+    int64_t          size = buffer->pVtbl->GetSize(buffer);
 
     if ((ret = ff_alloc_packet2(avctx, pkt, size, 0)) < 0) {
         return ret;
@@ -497,20 +492,7 @@  static int amf_copy_buffer(AVCodecContext *avctx, AVPacket *pkt, AMFBuffer *buff
 // amfenc API implementation
 int ff_amf_encode_init(AVCodecContext *avctx)
 {
-    AmfContext     *ctx = avctx->priv_data;
-    int             ret;
-
-    ctx->factory = NULL;
-    ctx->debug = NULL;
-    ctx->trace = NULL;
-    ctx->context = NULL;
-    ctx->encoder = NULL;
-    ctx->library = NULL;
-    ctx->version = 0;
-    ctx->eof = 0;
-    ctx->format = 0;
-    ctx->tracer.vtbl = NULL;
-    ctx->tracer.avctx = NULL;
+    int ret;
 
     if ((ret = amf_load_library(avctx)) == 0) {
         if ((ret = amf_init_context(avctx)) == 0) {
@@ -595,10 +577,10 @@  static void amf_release_buffer_with_frame_ref(AMFBuffer *frame_ref_storage_buffe
 
 int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
 {
-    AMF_RESULT      res = AMF_OK;
-    AmfContext     *ctx = avctx->priv_data;
-    AMFSurface     *surface = NULL;
-    int             ret;
+    AmfContext *ctx = avctx->priv_data;
+    AMFSurface *surface;
+    AMF_RESULT  res;
+    int         ret;
 
     if (!ctx->encoder)
         return AVERROR(EINVAL);