On Thu, Feb 28, 2013 at 3:16 PM, Diego Biurrun <[email protected]> wrote:
> 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. > > question about having config.h included I think we have the agreement : if moved to qsv.c with no HAVE_AV_CONFIG_H - fine, correct? > > > #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? > > currently, the implementation supports only Windows as the target OS. This is per MediaSDK support, therefore compilations only with MSVC and/or cross compilation with MinGW. > > > > > > > +#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. > > please see my very last comment, QSV is more than decode and its acceleration, includes VPP/filters and encode... This initial patch is QSV decode , more to follow... > > you can have a look on MediaSDK samples and definitions been used there, > > files like sample_common\include\sample_defs.h > > URL? > Homepage of MediaSDK is http://software.intel.com/en-us/vcsource/tools/media-sdk I can recommend to download and install MediaSDK to see this file. > > > > > > > > --- /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 > please note: regardless the fact that this, an initial patch provides QSV-based decode only - it has already "looking forward" structure and implementation. QSV based acceleration (and MediaSDK API ) goes beyond decode and only, it covers filters/VPP cases and encode - meaning: full video transcode scenario. I would encourage to read more from : http://software.intel.com/en-us/vcsource/tools/media-sdk and provided together with MediaSDK documentations. if an application does video transcode or any decode/encode : for best performance with QSV - we have to recommend to use not only libav/QSV decode but QSV based encode/filters as well. Such, fully HW accelerated, approach probably something new for libav but provided patch and its definitions and structures is a start to build such possibility as within libav and up to the final application. Note, it goes beyond libav at the level of final application. This is what I mean by "looking forward". About possible concern: to have only needed definitions etc - just believe that it will stay so but just extended in the future. Regards Maxym _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
