On 13-02-27 19:50:51, Diego Biurrun wrote:
> On Wed, Feb 27, 2013 at 06:39:57PM +0100, maxd wrote:
> > On 13-02-21 16:03:30, Diego Biurrun wrote:
> > > On Mon, Feb 04, 2013 at 08:49:35PM +0100, Maxym Dmytrychenko wrote:
> > > > From: Maxym Dmytrychenko <[email protected]>
> > > >
> > > Does your patch pass "make check"? I suspect at least the checkheaders
> > > target will not pass.
> >
> > my latest patch from
> > http://lists.libav.org/pipermail/libav-devel/2013-February/043882.html
> > should be fine,
> > including make checkheaders on MinGW and MSVC
>
> It only passes checkheaders if the mxf headers are available.
> Fixed in the changes I sent you.
got it and appriciated!
> > > > --- /dev/null
> > > > +++ b/libavcodec/qsv.c
> > > > @@ -0,0 +1,556 @@
> > > > +void av_qsv_stage_clean(av_qsv_stage ** stage)
> > > > +{
> > > > +
> > > > + if ((*stage)->out.p_sync) {
> > > > + *(*stage)->out.p_sync = 0;
> > > > + (*stage)->out.p_sync = 0;
> > > > + }
> > > > + av_freep(stage);
> > >
> > > Here a variable indirection would make things more readable.
> >
> > if you have any sample that you can show to follow, I can change it.
>
> av_qsv_stage *stage_ptr = *stage;
>
> Maybe use a better variable name.
ok, will change in the next patch
> > > > +av_qsv_stage *av_qsv_get_by_mask(av_qsv_list * list, int mask,
> > > > av_qsv_stage ** prev,
> > > > + av_qsv_list ** this_pipe)
> > > > +{
> > > > + av_qsv_list *item = 0;
> > > > + av_qsv_stage *stage = 0;
> > > > + av_qsv_stage *prev_stage = 0;
> > > > + int i = 0;
> > > > + int a = 0;
> > > > + *this_pipe = 0;
> > > > + *prev = 0;
> > > > + for (i = 0; i < av_qsv_list_count(list); i++) {
> > > > + item = av_qsv_list_item(list, i);
> > > > + for (a = 0; a < av_qsv_list_count(item); a++) {
> > > > + stage = av_qsv_list_item(item, a);
> > > > + if (!stage)
> > > > + return stage;
> > > > + if (stage->type & mask) {
> > > > + *prev = prev_stage;
> > > > + *this_pipe = item;
> > > > + return stage;
> > > > + }
> > > > + prev_stage = stage;
> > > > + }
> > > > + }
> > > > + return 0;
> > > > +}
> > >
> > > prev and this_pipe are only used dereferenced, so why not pass them as
> > > simple pointers?
> >
> > this way looks to be better for the final application that would use it
>
> Why?
ok , let's make it easy - I've removed this function completely.
> > > > --- /dev/null
> > > > +++ b/libavcodec/qsv.h
> > > > @@ -0,0 +1,469 @@
> > > You start the sentence with "As ..." but then you do not say what follows
> > > from the ability to use hw acceleration.
> > >
> > > > +#ifdef HAVE_AV_CONFIG_H
> > > > +#include "config.h"
> > > > +#endif
> > >
> > > No such inclusion guard is necessary.
> > it looks that might help to the final application ( dependency point as
> > it was last time )
> > I can check more here if needed.
> > If I will remove it from this file - will be a transparent change
> > for QSV implementation inside libav and final application.
>
> You cannot include config.h in an installed header, with or without the
> inclusion guards.
ok, if to move into qsv.c - will it be fine ?
if so, next patch will move lines:
#ifdef HAVE_AV_CONFIG_H
#include "config.h"
#endif
#if HAVE_THREADS
#if defined (__GNUC__)
#include <pthread.h>
#define ff_qsv_atomic_inc(ptr) __sync_add_and_fetch(ptr,1)
#define ff_qsv_atomic_dec(ptr) __sync_sub_and_fetch (ptr,1)
#elif HAVE_WINDOWS_H // MSVC case
#include <windows.h>
#if HAVE_PTHREADS
#include <pthread.h>
#elif HAVE_W32THREADS
#include "w32pthreads.h"
#endif
#define ff_qsv_atomic_inc(ptr) InterlockedIncrement(ptr)
#define ff_qsv_atomic_dec(ptr) InterlockedDecrement (ptr)
#else
// targeting only for MinGW or MSVC
#endif
#else
#define ff_qsv_atomic_inc(ptr) ((*ptr)++)
#define ff_qsv_atomic_dec(ptr) ((*ptr)--)
#endif
into qsv.c
> > > > +#define AV_QSV_ZERO_MEMORY(VAR) {memset(&VAR, 0,
> > > > sizeof(VAR));}
> > > > +#define AV_QSV_ALIGN32(X) (((mfxU32)((X)+31)) &
> > > > (~ (mfxU32)31))
> > > > +#define AV_QSV_ALIGN16(value) (((value + 15) >> 4) <<
> > > > 4)
> > > > +#ifndef AV_QSV_PRINT_RET_MSG
> > > > +#define AV_QSV_PRINT_RET_MSG(ERR) { av_log(NULL,
> > > > AV_LOG_FATAL,"Error code %d,\t%s\t%d\n", ERR, __FUNCTION__, __LINE__); }
> > > > +#endif
> > > > +
> > > > +#ifndef AV_QSV_DEBUG_ASSERT
> > > > +#define AV_QSV_DEBUG_ASSERT(x,y) {if ((x)) {av_log(NULL,
> > > > AV_LOG_FATAL,"\nASSERT: %s\n",y);};}
> > > > +#endif
> > > > +
> > > > +#define AV_QSV_CHECK_RESULT(P, X, ERR) {if ((X) > (P))
> > > > {AV_QSV_PRINT_RET_MSG(ERR); return ERR;}}
> > > > +#define AV_QSV_CHECK_POINTER(P, ERR) {if (!(P))
> > > > {AV_QSV_PRINT_RET_MSG(ERR); return ERR;}}
> > > > +#define AV_QSV_IGNORE_MFX_STS(P, X) {if ((X) == (P)) {P
> > > > = MFX_ERR_NONE;}}
> > > > +
> > > > +#define AV_QSV_ID_BUFFER MFX_MAKEFOURCC('B','U','F','F')
> > > > +#define AV_QSV_ID_FRAME MFX_MAKEFOURCC('F','R','M','E')
> > > > +
> > > > +#define AV_QSV_SURFACE_NUM 80
> > > > +#define AV_QSV_SYNC_NUM AV_QSV_SURFACE_NUM*3/4
> > > > +#define AV_QSV_BUF_SIZE_DEFAULT 4096*2160*10
> > > > +#define AV_QSV_JOB_SIZE_DEFAULT 10
> > > > +#define AV_QSV_SYNC_TIME_DEFAULT 10000
> > > > +// see av_qsv_get_free_sync, av_qsv_get_free_surface , 100 if
> > > > usleep(10*1000)(10ms) == 1 sec
> > > > +#define AV_QSV_REPEAT_NUM_DEFAULT 100
> > > > +#define AV_QSV_ASYNC_DEPTH_DEFAULT 4
> > > > +
> > > > +// version of MSDK/QSV API currently used
> > > > +#define AV_QSV_MSDK_VERSION_MAJOR 1
> > > > +#define AV_QSV_MSDK_VERSION_MINOR 3
> > >
> > > How much of all this needs to be in an installed header?
> > > It looks mostly like internal stuff that needs not be part of externally
> > > visible API to me. The same applies to the rest of this header.
> > very much yes,
> > as for the final application that will use libav and QSV,
> > just remember that QSV is decode/filters/encode - full pipeline,
> > therefore has to be properly build and operated.
>
> All of these look like implementation details that should not be exposed
> to a calling application. Some of it like AV_QSV_ZERO_MEMORY looks bogus.
not to go one by one with each line :
- these lines will be used by final applications to set OR to get details of
QSV workflow.
- it would be important to keep AV_QSV_CHECK_... , AV_QSV_ALIGN / ZERO etc
like a bridge for developers, as MediaSDK samples have quite
similar structure and naming - therefore it will definetly help in
adoption and usage : MSDK samples and libav.
> > > > --- /dev/null
> > > > +++ b/libavcodec/qsv_h264.c
> > > > @@ -0,0 +1,920 @@
> > > > + qsv->impl = qsv_config_context->impl_requested;
> > > > +
> > > > + memset(&qsv->mfx_session, 0, sizeof(mfxSession));
> > > > + qsv->ver.Major = AV_QSV_MSDK_VERSION_MAJOR;
> > > > + qsv->ver.Minor = AV_QSV_MSDK_VERSION_MINOR;
> > > > +
> > > > + sts = MFXInit(qsv->impl, &qsv->ver, &qsv->mfx_session);
> > > > + AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
> > > > +
> > > > + AV_QSV_ZERO_MEMORY(qsv_decode->m_mfxVideoParam);
> > > > + AV_QSV_ZERO_MEMORY(qsv_decode->m_mfxVideoParam.mfx);
> > > > + qsv_decode->m_mfxVideoParam.mfx.CodecId = MFX_CODEC_AVC;
> > > > + qsv_decode->m_mfxVideoParam.IOPattern =
> > > > + qsv_config_context->io_pattern;
> > > > +
> > > > + qsv_decode->m_mfxVideoParam.AsyncDepth =
> > > > + qsv_config_context->async_depth;
> > > > +
> > > > + AV_QSV_ZERO_MEMORY(qsv_decode->bs);
> > >
> > > I suspect it would be simpler if you just zeroed all of qsv/qsv_decode
> > > at initialization - is there a reason not to?
> > good point , see my latest patch
>
> Your latest patch makes no changes to this block.
http://lists.libav.org/pipermail/libav-devel/2013-February/043882.html
it is changed into:
+ sts = MFXInit(qsv->impl, &qsv->ver, &qsv->mfx_session);
+ AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+
+ AV_QSV_ZERO_MEMORY(qsv_decode->m_mfxVideoParam);
+ AV_QSV_ZERO_MEMORY(qsv_decode->bs);
+ qsv_decode->m_mfxVideoParam.mfx.CodecId = MFX_CODEC_AVC;
+ qsv_decode->m_mfxVideoParam.IOPattern =
qsv_config_context->io_pattern;
+
+ qsv_decode->m_mfxVideoParam.AsyncDepth =
qsv_config_context->async_depth;
> > > > + qsv_decode->request[0].Type |= MFX_MEMTYPE_SYSTEM_MEMORY;
> > > > +
> > > > + qsv_config_context->allocators->
> > > > + frame_alloc.Alloc(qsv_config_context->allocators,
> > > > + &qsv_decode->request[0],
> > > > + &qsv_decode->response);
> > > > + }
> > > > +
> > > > + for (int i = 0; i < qsv_decode->surface_num; i++) {
> > > > + qsv_decode->p_surfaces[i] =
> > > > av_mallocz(sizeof(mfxFrameSurface1));
> > > > + AV_QSV_CHECK_POINTER(qsv_decode->p_surfaces[i],
> > > > + AVERROR(ENOMEM));
> > >
> > > I'm not convinced of this error checking with macros.
> > > IMO it does not help readability or anything else really.
> >
> > it should just return libav error ENOMEM if pointer is zero
>
> I understand what it does; I just think it is silly to use a macro for it.
> IMO it's just unnecessary obfuscation and inconsistent with the rest of
> the codebase.
...CHECK_POINTER is one of the approach in MediaSDK samples etc
if no strong objection(s) - I would recommend to keep it as a "bridge"
(see above, in my previous answers about MSDK samples)
> > > > +static int qsv_decode_frame(AVCodecContext * avctx, void *data,
> > > > + int *data_size, AVPacket * avpkt)
> > > > +{
> > > > + uint8_t *current_position = avpkt->data;
> > > > +
> > > > + AVFrame *picture = (AVFrame *) data;
> > >
> > > pointless void* cast
> >
> > in C - still fine to remove, C++ might give a big problem ,
> > see my latest patch
>
> This is very much a C project. Please remove all the void* casts, more
> are left in your patch.
it is fine to stick to C style.
should be part of the next patch
> > it is quite long mail - hopefully all questions have answers,
> > most changes and they are implemented already at the recent patch :
> > http://lists.libav.org/pipermail/libav-devel/2013-February/043882.html
> >
> > if you would consider to change something - feel free to do it.
> >
> > Let me to know if any other question(s)
>
> Just see the comments I had above.
>
> Diego
> _______________________________________________
> libav-devel mailing list
> [email protected]
> https://lists.libav.org/mailman/listinfo/libav-devel
any more comments?
let me to know if new patch should be created.
I would like to include all feedback(s) into it.
regards
Maxym
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
