On Thu, Feb 28, 2013 at 02:47:39PM +0100, Max Dm wrote:
> On Thu, Feb 28, 2013 at 11:38 AM, Diego Biurrun <[email protected]> wrote:
> > On Wed, Feb 27, 2013 at 11:46:45PM +0100, Maxym Dmytrychenko wrote:
> > > 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:
> > > > > > > --- /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
> >
> > Again - no such inclusion guard is necessary.
> >
> > right - it was just to show the idea and what is moved,
> #ifdef HAVE_AV_CONFIG_H will be dropped while in qsv.c
It has to be and had to be dropped anywhere.
> Again, will such move help to solve the question?
Not sure which question you are referring to.
> > #define ff_qsv_atomic_inc(ptr) __sync_add_and_fetch(ptr,1)
> > > #define ff_qsv_atomic_dec(ptr) __sync_sub_and_fetch (ptr,1)
> > > #define ff_qsv_atomic_inc(ptr) InterlockedIncrement(ptr)
> > > #define ff_qsv_atomic_dec(ptr) InterlockedDecrement (ptr)
> >
> > stray formatting
>
> formatting is going to be as per latest patch,
> here is might be an email issue(?)
> if not - please explain: "stray formatting", an example ?
some lines w/, some w/o space before (; also, space after comma
> > > #else
> > > // targeting only for MinGW or MSVC
> > > #endif
> >
> > ?
> >
> targeting MSVC and MinGW only
> if you dont like to - please drop
This does not work for other platforms?
> > > > > > > +#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.
> >
> > How does having to learn a new macro for setting a variable to zero help
> > anybody, much less make things easier to handle?
>
> Try to see it a bit wide: if you are developer for the final application
> and use libav/QSV and MediaSDK samples as reference.
> if all parts have the own way of doing the same things - it would be not a
> good idea.
>
> if we will help to such developer by having similarity - it will
> be beneficial for all.
Yes, that's why a developer using libav to develop, say, VLC, should not
have to implement acceleration through qsv in one way, through vdpau in
another and through xyz in a third way.
I don't see how a developer dealing with qsv through libav will have to
deal with qsv more than with libav.
> you can have a look on MediaSDK samples and definitions been used there,
> files like sample_common\include\sample_defs.h
URL?
> > > > > > > --- /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;
> >
> > And I can only repeat what I said before:
> >
> > I suspect it would be simpler if you just zeroed all of qsv/qsv_decode
> > at initialization - is there a reason not to?
> >
> > Now I've got you question and in details.
>
> MSDK usage would recommend to be sure that structures are zeroed.
>
> as it is now in libav:
>
> qsv = av_mallocz(sizeof(av_qsv_context));
>
> qsv_decode = av_mallocz(sizeof(av_qsv_space));
>
> qsv_decode->p_buf_max_size = AV_QSV_BUF_SIZE_DEFAULT;
> qsv_decode->p_buf =
> av_malloc_array(qsv_decode->p_buf_max_size, sizeof(uint8_t));
>
> so, I might remove
>
> + AV_QSV_ZERO_MEMORY(qsv_decode->m_mfxVideoParam);
> + AV_QSV_ZERO_MEMORY(qsv_decode->bs);
>
>
> would it help and close the issue?
Never mind, I think I misread.
> > > > > > + 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)
> >
> > libav is not the media SDK you speak about - why should we adopt such
> > macros to check memory allocations in one file when we have hundreds
> > of memory allocations checked w/o such a macro?
>
> please see the situation wide:
> QSV usage goes outside libav into final application,
> I am not sure that some QSV filter/VPP component inside the final
> application and been outside of libav must use additional/own macro
> from somewhere therefore - I just recommend to stick to defined here.
>
> if we would like to ( and technically ) - let's just map it into libav's
> one , using the macro should be fine for this.
Sorry, I cannot parse this paragraph.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
