> -----Original Message----- > From: Kravchenko, Alexander > Sent: Tuesday, February 27, 2018 2:47 PM > To: libav development > Subject: RE: [libav-devel] [PATCH] AMF SDK integration code cleanup: > remove writer_id option & move AMF_COMMON_OPTIONS out from > amfenc.h > > Comments are inline > > > -----Original Message----- > > From: libav-devel [mailto:[email protected]] On Behalf Of > Diego > > Biurrun > > Sent: Tuesday, February 27, 2018 2:11 PM > > To: libav development > > Subject: Re: [libav-devel] [PATCH] AMF SDK integration code cleanup: > > remove writer_id option & move AMF_COMMON_OPTIONS out from > > amfenc.h > > > > This patch accmoplishes multiple unrelated things. Ideally you would > submit > > one patch per change. Also, there is no explanation of why you are doing > > these changes in the commit message. > > - Hi, I am collaborating with AMD on AMF integration tasks. Probably I should > start in thread: "[libav-devel] Add HW H.264 and HEVC encoding for AMD > GPUs based on AMF SDK" > > - I have put changes to one patch, because all of them solves 'options' > cleanup task. If it is required can split this to the following separated > changes > 1. Remove writer_id > 2. move AMF_COMMON_OPTIONS out from amfenc.h > > > > > On Tue, Feb 27, 2018 at 10:34:27AM +0000, Kravchenko, Alexander wrote: > > > From 4d0efe3f5fd03db188f41d52ee9549a046939d1d Mon Sep 17 00:00:00 > > 2001 > > > From: diamond88 <[email protected]> > > > > Looks like your Git is misconfigured, please set your name properly. > > Ok > > > > > > --- a/libavcodec/amfenc.h > > > +++ b/libavcodec/amfenc.h > > > @@ -151,8 +148,4 @@ extern const enum AVPixelFormat > > ff_amf_pix_fmts[]; > > > > > > -#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 } \ > > > - > > > --- a/libavcodec/amfenc_h264.c > > > +++ b/libavcodec/amfenc_h264.c > > > > > > - 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 }, > > > > > > --- a/libavcodec/amfenc_hevc.c > > > +++ b/libavcodec/amfenc_hevc.c > > > @@ -92,7 +91,7 @@ static const AVOption options[] = { > > > > > > - 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 }, > > > > Why is this an improvement? > > - Current code cause warning described here > https://lists.libav.org/pipermail/libav-devel/2018-February/085705.html > > ---------- > /home/diego/src/libav.vanilla/libavcodec/amfenc.c: In function > ‘amf_init_context’: > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:159:51: warning: passing > argument 2 of ‘ctx->trace->pVtbl->RegisterWriter’ from incompatible pointer > type [-Wincompatible-pointer-types] > ctx->trace->pVtbl->RegisterWriter(ctx->trace, ctx->writer_id, > (AMFTraceWriter *)&ctx->tracer, 1); > ^~~ > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:159:51: note: expected > ‘const wchar_t * {aka const int *}’ but argument is of type ‘char *’ > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:160:51: warning: passing > argument 2 of ‘ctx->trace->pVtbl->SetWriterLevel’ from incompatible > pointer type [-Wincompatible-pointer-types] > ctx->trace->pVtbl->SetWriterLevel(ctx->trace, ctx->writer_id, > AMF_TRACE_TRACE); > ^~~ > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:160:51: note: expected > ‘const wchar_t * {aka const int *}’ but argument is of type ‘char *’ > /home/diego/src/libav.vanilla/libavcodec/amfenc.c: In function > ‘ff_amf_encode_close’: > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:265:57: warning: passing > argument 2 of ‘ctx->trace->pVtbl->UnregisterWriter’ from incompatible > pointer type [-Wincompatible-pointer-types] > ctx->trace->pVtbl->UnregisterWriter(ctx->trace, ctx->writer_id); > ^~~ > /home/diego/src/libav.vanilla/libavcodec/amfenc.c:265:57: note: expected > ‘const wchar_t * {aka const int *}’ but argument is of type ‘char *’ > ---------- > > - I don’t see common practice to configure component logging like this. It > look like this should be done by another way (config file in working dir or > some other way) > This will allow to have interface clean > > > > > Diego > > _______________________________________________ > > libav-devel mailing list > > [email protected] > > https://lists.libav.org/mailman/listinfo/libav-devel > >
I missed description about AMF_COMMON_OPTIONS movement to out of AMF_COMMON_OPTIONS out from amfenc.h It look like It is not required to have one common option declaration for two components in header. This makes code less readable, but does not provide much benefits. ________________________________ This e-mail and any attachment(s) are intended only for the recipient(s) named above and others who have been specifically authorized to receive them. They may contain confidential information. If you are not the intended recipient, please do not read this email or its attachment(s). Furthermore, you are hereby notified that any dissemination, distribution or copying of this e-mail and any attachment(s) is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender by replying to this e-mail and then delete this e-mail and any attachment(s) or copies thereof from your system. Thank you. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
