AMF SDK integration code cleanup: remove writer_id option & move AMF_COMMON_OPTIONS out from amfenc.h

Message ID 1006FD0F0FE06E479CEE8E19981AE07036804178@ORO-MBOX-05.luxoft.com
State New
Headers show
Series
  • AMF SDK integration code cleanup: remove writer_id option & move AMF_COMMON_OPTIONS out from amfenc.h
Related show

Commit Message

Kravchenko, Alexander Feb. 27, 2018, 10:51 a.m.
From 4d0efe3f5fd03db188f41d52ee9549a046939d1d Mon Sep 17 00:00:00 2001
From: diamond88 <mr.aleksandr.kravchenko@gmail.com>
Date: Tue, 27 Feb 2018 09:46:08 +0300
Subject: [PATCH] AMF SDK integration code cleanup: remove writer_id option &
 move AMF_COMMON_OPTIONS out from amfenc.h

---
 libavcodec/amfenc.c      | 12 ++++++++----
 libavcodec/amfenc.h      |  7 -------
 libavcodec/amfenc_h264.c |  3 +--
 libavcodec/amfenc_hevc.c |  3 +--
 4 files changed, 10 insertions(+), 15 deletions(-)

--
2.16.2.windows.1

Comments

Luca Barbato Feb. 27, 2018, 12:28 p.m. | #1
On 27/02/2018 11:51, Kravchenko, Alexander wrote:
>  From 4d0efe3f5fd03db188f41d52ee9549a046939d1d Mon Sep 17 00:00:00 2001
> From: diamond88 <mr.aleksandr.kravchenko@gmail.com>
> Date: Tue, 27 Feb 2018 09:46:08 +0300
> Subject: [PATCH] AMF SDK integration code cleanup: remove writer_id option &
>   move AMF_COMMON_OPTIONS out from amfenc.h
> 

Hi, I thought the library user would like to have a separate writer_id 
since I assumed that the identifier is used for logging and resource 
accounting purposes (e.g. browsers using this backend might want to use 
a separate identifier per-tab).

The common options pattern is used when there are multiple codecs 
relying on the same backend. I would expect to have the common options 
expanded instead of dropped, what are your plans exactly?

lu
Kravchenko, Alexander Feb. 27, 2018, 1:59 p.m. | #2
>
> Hi, I thought the library user would like to have a separate writer_id
> since I assumed that the identifier is used for logging and resource
> accounting purposes (e.g. browsers using this backend might want to use
> a separate identifier per-tab).
>
> The common options pattern is used when there are multiple codecs
> relying on the same backend. I would expect to have the common options
> expanded instead of dropped, what are your plans exactly?
>
> lu
>

Hi,

1) AMFTraceWriter is abstraction to configure how AMF outputs its logs for current process, not for component.

Example instances of AMFTraceWriter can be
* FileWriter
* SocketWriter
* DebugOutputWriter
* LibavWriter (output using av_log function).

AMFTraceWriter can be Registered/Unregistered/Enabled/Disabled and configured to output particular level of trace output.

If we use multiple LibavWriter objects in one process, we will have duplication of output in avlib log.
To prevent this scenario we should use one constant writer_id .

2) AMF_COMMON_OPTIONS in header. It is more about balance between preventing duplication and code readability and visibility.
IMHO minor duplication is less important than core readability and visibility. I'd recommend to have property map for each component in one place.
Mironov, Mikhail Feb. 27, 2018, 2:53 p.m. | #3
> -----Original Message-----
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> Kravchenko, Alexander
> Sent: February 27, 2018 8:59 AM
> To: libav development <libav-devel@libav.org>
> Subject: Re: [libav-devel] [PATCH] AMF SDK integration code cleanup: remove
> writer_id option & move AMF_COMMON_OPTIONS out from amfenc.h
> 
> >
> > Hi, I thought the library user would like to have a separate writer_id
> > since I assumed that the identifier is used for logging and resource
> > accounting purposes (e.g. browsers using this backend might want to
> > use a separate identifier per-tab).
> >
> > The common options pattern is used when there are multiple codecs
> > relying on the same backend. I would expect to have the common options
> > expanded instead of dropped, what are your plans exactly?
> >
> > lu
> >
> 
> Hi,
> 
> 1) AMFTraceWriter is abstraction to configure how AMF outputs its logs for
> current process, not for component.
> 
> Example instances of AMFTraceWriter can be
> * FileWriter
> * SocketWriter
> * DebugOutputWriter
> * LibavWriter (output using av_log function).
> 
> AMFTraceWriter can be Registered/Unregistered/Enabled/Disabled and
> configured to output particular level of trace output.
> 
> If we use multiple LibavWriter objects in one process, we will have
> duplication of output in avlib log.
> To prevent this scenario we should use one constant writer_id .
> 
> 2) AMF_COMMON_OPTIONS in header. It is more about balance between
> preventing duplication and code readability and visibility.
> IMHO minor duplication is less important than core readability and visibility.
> I'd recommend to have property map for each component in one place.
> 
> 
To address the issue of uniquely identifying a component/codec/filter shouldn't we add some common mechanism? 
Imagine that an app has two x264 encoders and wants to distinguish traces for each one. 
If every component will have own system of identification it will be very hard to use.
Mikhail
Luca Barbato Feb. 27, 2018, 5:09 p.m. | #4
On 27/02/2018 14:59, Kravchenko, Alexander wrote:> If we use multiple 
LibavWriter objects in one process, we will have
> duplication of output in avlib log. To prevent this scenario we
> should use one constant writer_id .

Thanks for the explanation, I'm fine with this change now.

lu
Kravchenko, Alexander Feb. 27, 2018, 6:24 p.m. | #5
> -----Original Message-----
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Luca
> Barbato
> Sent: Tuesday, February 27, 2018 8:10 PM
> To: libav-devel@libav.org
> Subject: Re: [libav-devel] [PATCH] AMF SDK integration code cleanup:
> remove writer_id option & move AMF_COMMON_OPTIONS out from
> amfenc.h
>
> On 27/02/2018 14:59, Kravchenko, Alexander wrote:> If we use multiple
> LibavWriter objects in one process, we will have
> > duplication of output in avlib log. To prevent this scenario we
> > should use one constant writer_id .
>
> Thanks for the explanation, I'm fine with this change now.
>
> lu
>
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel


What are the next steps?

Are you going to submit patch? Or should I create new one, containing only this change?
Luca Barbato Feb. 27, 2018, 7:39 p.m. | #6
On 27/02/2018 19:24, Kravchenko, Alexander wrote:
>> -----Original Message-----
>> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Luca
>> Barbato
>> Sent: Tuesday, February 27, 2018 8:10 PM
>> To: libav-devel@libav.org
>> Subject: Re: [libav-devel] [PATCH] AMF SDK integration code cleanup:
>> remove writer_id option & move AMF_COMMON_OPTIONS out from
>> amfenc.h
>>
>> On 27/02/2018 14:59, Kravchenko, Alexander wrote:> If we use multiple
>> LibavWriter objects in one process, we will have
>>> duplication of output in avlib log. To prevent this scenario we
>>> should use one constant writer_id .
>>
>> Thanks for the explanation, I'm fine with this change now.
>>
>> lu
>>
>> _______________________________________________
>> libav-devel mailing list
>> libav-devel@libav.org
>> https://lists.libav.org/mailman/listinfo/libav-devel
> 
> 
> What are the next steps?
> 
> Are you going to submit patch? Or should I create new one, containing only this change?
> 

I'd wait ~24h so people could comment and then I'll merge your change, 
no need to do more work on that :)

For the other change I guess mainly depends on how many other options 
that are shared across hevc and h264 you'll expose in the future.

the patch with just the writer_id changed with the following message:

"
amf_enc: Remove the writer_id option

The writer id is a logger identifier and there is only one: av_log.
"

lu
Diego Biurrun Feb. 28, 2018, 12:14 a.m. | #7
On Tue, Feb 27, 2018 at 08:39:48PM +0100, Luca Barbato wrote:
> On 27/02/2018 19:24, Kravchenko, Alexander wrote:
> > > -----Original Message-----
> > > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Luca
> > > Barbato
> > > Sent: Tuesday, February 27, 2018 8:10 PM
> > > To: libav-devel@libav.org
> > > Subject: Re: [libav-devel] [PATCH] AMF SDK integration code cleanup:
> > > remove writer_id option & move AMF_COMMON_OPTIONS out from
> > > amfenc.h
> > > 
> > > On 27/02/2018 14:59, Kravchenko, Alexander wrote:> If we use multiple
> > > LibavWriter objects in one process, we will have
> > > > duplication of output in avlib log. To prevent this scenario we
> > > > should use one constant writer_id .
> > > 
> > > Thanks for the explanation, I'm fine with this change now.
> > 
> > What are the next steps?
> > 
> > Are you going to submit patch? Or should I create new one, containing only this change?
> > 
> 
> I'd wait ~24h so people could comment and then I'll merge your change, no
> need to do more work on that :)

Please send fresh patches for review instead, there's too many issues with
the current one to be able to guess what you plan to merge.

Diego

Patch

diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
index f305a48bf..f369ae68b 100644
--- a/libavcodec/amfenc.c
+++ b/libavcodec/amfenc.c
@@ -20,10 +20,11 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */

+#include "config.h"
+
 #include "libavutil/avassert.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/hwcontext.h"
-#include "internal.h"
 #if CONFIG_D3D11VA
 #include "libavutil/hwcontext_d3d11va.h"
 #endif
@@ -32,6 +33,7 @@ 
 #include "libavutil/time.h"

 #include "amfenc.h"
+#include "internal.h"

 #if CONFIG_D3D11VA
 #include <d3d11.h>
@@ -46,6 +48,8 @@ 
 #include <dlfcn.h>
 #endif

+#define LIBAV_AMF_WRITER_ID L"libav_amf"
+
 #define PTS_PROP L"PtsProp"

 const enum AVPixelFormat ff_amf_pix_fmts[] = {
@@ -171,8 +175,8 @@  static int amf_init_context(AVCodecContext *avctx)
     // connect AMF logger to av_log
     ctx->tracer.vtbl = &tracer_vtbl;
     ctx->tracer.avctx = avctx;
-    ctx->trace->pVtbl->RegisterWriter(ctx->trace, ctx->writer_id, (AMFTraceWriter*)&ctx->tracer, 1);
-    ctx->trace->pVtbl->SetWriterLevel(ctx->trace, ctx->writer_id, AMF_TRACE_TRACE);
+    ctx->trace->pVtbl->RegisterWriter(ctx->trace, LIBAV_AMF_WRITER_ID,(AMFTraceWriter*)&ctx->tracer, 1);
+    ctx->trace->pVtbl->SetWriterLevel(ctx->trace, LIBAV_AMF_WRITER_ID, AMF_TRACE_TRACE);

     res = ctx->factory->pVtbl->CreateContext(ctx->factory, &ctx->context);
     AMF_RETURN_IF_FALSE(ctx, res == AMF_OK, AVERROR_UNKNOWN, "CreateContext() failed with error %d\n", res);
@@ -283,7 +287,7 @@  int av_cold ff_amf_encode_close(AVCodecContext *avctx)
     av_buffer_unref(&ctx->hw_frames_ctx);

     if (ctx->trace) {
-        ctx->trace->pVtbl->UnregisterWriter(ctx->trace, ctx->writer_id);
+        ctx->trace->pVtbl->UnregisterWriter(ctx->trace, LIBAV_AMF_WRITER_ID);
     }
     if (ctx->library) {
         dlclose(ctx->library);
diff --git a/libavcodec/amfenc.h b/libavcodec/amfenc.h
index f3b82be77..b3b582d97 100644
--- a/libavcodec/amfenc.h
+++ b/libavcodec/amfenc.h
@@ -20,7 +20,6 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */

-
 #ifndef AVCODEC_AMFENC_H
 #define AVCODEC_AMFENC_H

@@ -31,7 +30,6 @@ 

 #include "libavutil/fifo.h"

-#include "config.h"
 #include "avcodec.h"


@@ -80,7 +78,6 @@  typedef struct AmfContext {

     // common encoder options
     int                 log_to_dbg;
-    char                *writer_id;

     // Static options, have to be set before Init() call
     int                 usage;
@@ -151,8 +148,4 @@  extern const enum AVPixelFormat ff_amf_pix_fmts[];
         return ret_value; \
     }

-#define AMF_COMMON_OPTIONS \
-    { "log_to_dbg",     "Enable AMF logging to debug output",   OFFSET(log_to_dbg), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE }, \
-    { "writer_id",      "Enable AMF logging to writer id",      OFFSET(writer_id),  AV_OPT_TYPE_STRING, { .str = "libavcodec" }, 0, 1, VE } \
-
 #endif //AVCODEC_AMFENC_H
diff --git a/libavcodec/amfenc_h264.c b/libavcodec/amfenc_h264.c
index 01b0c3a56..74535784c 100644
--- a/libavcodec/amfenc_h264.c
+++ b/libavcodec/amfenc_h264.c
@@ -123,7 +123,7 @@  static const AVOption options[] = {

     { "aud",            "Inserts AU Delimiter NAL unit",        OFFSET(aud)          ,AV_OPT_TYPE_INT,  { .i64 = 0 }, 0, 1, VE },

-    AMF_COMMON_OPTIONS,
+    { "log_to_dbg",     "Enable AMF logging to debug output",   OFFSET(log_to_dbg)    , AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE },

     { NULL }
 };
@@ -217,7 +217,6 @@  static av_cold int amf_encode_init_h264(AVCodecContext *avctx)
         }
     }

-
     if (ctx->rate_control_mode == AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_CONSTANT_QP) {
         AMF_ASSIGN_PROPERTY_INT64(res, ctx->encoder, AMF_VIDEO_ENCODER_RATE_CONTROL_PREANALYSIS_ENABLE, AMF_VIDEO_ENCODER_PREENCODE_DISABLED);
         if (ctx->preanalysis)
diff --git a/libavcodec/amfenc_hevc.c b/libavcodec/amfenc_hevc.c
index fc64decde..151ca85bd 100644
--- a/libavcodec/amfenc_hevc.c
+++ b/libavcodec/amfenc_hevc.c
@@ -20,7 +20,6 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */

-
 #include "libavutil/internal.h"
 #include "libavutil/opt.h"
 #include "amfenc.h"
@@ -92,7 +91,7 @@  static const AVOption options[] = {

     { "aud",            "Inserts AU Delimiter NAL unit",            OFFSET(aud)           ,AV_OPT_TYPE_INT,{ .i64 = 0 }, 0, 1, VE },

-    AMF_COMMON_OPTIONS,
+    { "log_to_dbg",     "Enable AMF logging to debug output",       OFFSET(log_to_dbg)    ,AV_OPT_TYPE_INT,{ .i64 = 0 }, 0, 1, VE },

     { NULL }
 };