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]> > > > > Provided feedback from Diego Biurrun, Thu Jan 31 14:38:45 CET 2013 and IRC > > disussion included > > as much as it is possible. > > > > For now: MinGW and MSVC compilers and targeting Windows OS. > > > > For compilation - expected structure from Intel Media SDK: > > lib/ > > libmfx.lib (or .a for MinGW case) > > msdk/ > > mfxdefs.h > > mfxjpeg.h > > mfxmvc.h > > mfxplugin.h > > mfxplugin++.h > > mfxstructures.h > > mfxvideo.h > > mfxvideo++.h > > > > Media SDK provides libmfx.lib, > > you can also build one from the provided at Media SDK 2013 sources. > > > > Intel Media SDK available at > > http://software.intel.com/en-us/vcsource/tools/media-sdk > > > > Usage example: > > MSVC case: > > ./configure --toolchain=msvc --extra-cflags="-I/d/hb/libav/msinttypes-r26 > > -I/d/hb/libav/MediaSDK_2013" > > --extra-ldflags="-libpath:d:/hb/libav/MediaSDK_2013/lib/" > > --extra-libs="libmfx.lib" --disable-shared --prefix=/d/hb/libav/libav_build > > --enable-qsv > > > > > > Note: > > Current c99wrap/c99conv(1.0.1) limitation with MSVC (WIP to fix): > > > > - header mfxstructures.h might look like: > > #define MFX_MAKEFOURCC(ch0, ch1, ch2, ch3) ((DWORD)(BYTE)(ch0) | > > ((DWORD)(BYTE)(ch1) << 8) | \ > > ((DWORD)(BYTE)(ch2) << 16) | > > ((DWORD)(BYTE)(ch3) << 24 )) > > instead of > > #define MFX_MAKEFOURCC(A,B,C,D) > > ((((int)A))+(((int)B)<<8)+(((int)C)<<16)+(((int)D)<<24)) > > > > --- > > Most of this should not be part of the log message, but rather be an > annotation, see the --annotate option of git-send-email or add notes > below the "---" if you use git-format-patch. ok, will consider it for the next patch Wanted to keep this info at some place
> > 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 > > --- a/Changelog > > +++ b/Changelog > > @@ -3,7 +3,7 @@ releases are sorted from youngest to oldest. > > > > version 10: > > - av_strnstr > > - > > +- QSV decoder hardware acceleration > > > > version 9: > > - av_basename and av_dirname > > The empty line was there on purpose. please see my latest patch, as above > > --- a/configure > > +++ b/configure > > @@ -129,6 +129,7 @@ Component options: > > --disable-rdft disable RDFT code > > --disable-fft disable FFT code > > --enable-dxva2 enable DXVA2 code > > + --enable-qsv enable QSV code > > --enable-vaapi enable VAAPI code > > --enable-vda enable VDA code > > --enable-vdpau enable VDPAU code > > @@ -1062,6 +1063,7 @@ CONFIG_LIST=" > > nonfree > > openssl > > pic > > + qsv > > rdft > > runtime_cpudetect > > safe_bitstream_reader > > This needs to be rebased on top of current master. please see my latest patch > > @@ -1608,6 +1611,7 @@ h263_vaapi_hwaccel_select="vaapi h263_decoder" > > h263_vdpau_hwaccel_select="vdpau h263_decoder" > > h264_dxva2_hwaccel_deps="dxva2api_h" > > h264_dxva2_hwaccel_select="dxva2 h264_decoder" > > +h264_qsv_decoder_select="qsv h264_decoder" > > h264_vaapi_hwaccel_select="vaapi h264_decoder" > > h264_vda_hwaccel_select="vda h264_decoder" > > h264_vdpau_decoder_select="vdpau h264_decoder" > > This is wrong, but all surrounding lines are wrong as well. It should > depend on qsv and select the h264 decoder. I have a patch locally to > fix this block. Rebase your patch on top once this is in master. please see my latest patch > > @@ -3578,6 +3582,11 @@ if ! disabled vdpau && enabled vdpau_vdpau_h; then > > > > +# check for MSDK headers > > +if ! disabled qsv && check_header msdk/mfxvideo.h; then > > + enable qsv > > +fi > > This is not how it's done, just check for the header, the dependency > system will take care of the rest. > see my latest patch > --- a/libavcodec/Makefile > > +++ b/libavcodec/Makefile > > @@ -306,6 +307,7 @@ OBJS-$(CONFIG_QCELP_DECODER) += qcelpdec.o > > \ > > OBJS-$(CONFIG_QDRAW_DECODER) += qdrw.o > > OBJS-$(CONFIG_QPEG_DECODER) += qpeg.o > > +OBJS-$(CONFIG_H264_QSV_DECODER) += qsv_h264.o qsv.o > > OBJS-$(CONFIG_QTRLE_DECODER) += qtrle.o > > OBJS-$(CONFIG_QTRLE_ENCODER) += qtrleenc.o > > --- a/libavcodec/allcodecs.c > > +++ b/libavcodec/allcodecs.c > > @@ -150,6 +150,7 @@ void avcodec_register_all(void) > > REGISTER_ENCODER(H263P, h263p); > > REGISTER_DECODER(H264, h264); > > + REGISTER_DECODER(H264_QSV, h264_qsv); > > REGISTER_DECODER(H264_VDPAU, h264_vdpau); > > REGISTER_ENCDEC (HUFFYUV, huffyuv); > > You should implement this as a proper hwaccel and not a decoder, but you > already know that. > > The comments below thus apply to code of which large parts may need to > be rewritten. I hope they are helpful nonetheless. as per several discussion - it looks that AVCodec based implementation is preferable > > --- /dev/null > > +++ b/libavcodec/qsv.c > > @@ -0,0 +1,556 @@ > > +/* ********************************************************************* *\ > > + > > +Copyright (C) 2013 Intel Corporation. All rights reserved. > > + > > +Redistribution and use in source and binary forms, with or without > > +modification, are permitted provided that the following conditions are met: > > +- Redistributions of source code must retain the above copyright notice, > > +this list of conditions and the following disclaimer. > > +- Redistributions in binary form must reproduce the above copyright notice, > > +this list of conditions and the following disclaimer in the documentation > > +and/or other materials provided with the distribution. > > +- Neither the name of Intel Corporation nor the names of its contributors > > +may be used to endorse or promote products derived from this software > > +without specific prior written permission. > > + > > +THIS SOFTWARE IS PROVIDED BY INTEL CORPORATION "AS IS" AND ANY EXPRESS OR > > +IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES > > +OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. > > +IN NO EVENT SHALL INTEL CORPORATION BE LIABLE FOR ANY DIRECT, INDIRECT, > > +INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT > > +NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > > +THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > + > > +\* ********************************************************************* */ > > nit: Please add "* " before all lines to make this look like all other > license headers. see my latest patch > > +#include "qsv.h" > > + > > +#include "avcodec.h" > > +#include "internal.h" > > + > > +int av_qsv_get_free_encode_task(av_qsv_list * tasks) > > *tasks > > There are many more instances below, please fix them. see my latest patch > > +int av_qsv_get_free_sync(av_qsv_space * space, av_qsv_context * qsv) > > +{ > > + int ret = -1; > > + int counter = 0; > > + > > + while (1) { > > + for (int i = 0; i < space->sync_num; i++) { > > + if (!(*(space->p_sync[i])) > > + && !ff_qsv_is_sync_in_pipe(space->p_sync[i], qsv)) { > > Keep && and || at the end of the line, more below. see my latest patch > > +#if HAVE_THREADS > > + if (++counter >= AV_QSV_REPEAT_NUM_DEFAULT) { > > +#endif > > + av_log(NULL, AV_LOG_FATAL, "not enough to have %d sync > > point(s) allocated\n", > > + space->sync_num); > > This sentence no verb. see my latest patch > > +int av_qsv_get_free_surface(av_qsv_space * space, av_qsv_context * qsv, > > + mfxFrameInfo * info, av_qsv_split part) > > Indentation is off, more below. see my latest patch > > > + for (int i = from; i < up; i++) { > > + if ((0 == space->p_surfaces[i]->Data.Locked) > > We generally write "!space->p_surfaces[i]->Data.Locked" instead. see my latest patch > > > + av_log(NULL, AV_LOG_FATAL, > > + "not enough to have %d surface(s) allocated\n", up); > > no verb again see my latest patch > > > + for (a = 0; a < av_qsv_list_count(qsv->pipes); a++) { > > + list = av_qsv_list_item(qsv->pipes, a); > > + for (b = 0; b < av_qsv_list_count(list); b++) { > > + stage = av_qsv_list_item(list, b); > > + if (sync == stage->out.p_sync) { > > + return 1; > > + } > > nit: pointless {} see my latest patch > > > +av_qsv_stage *av_qsv_stage_init(void) > > +{ > > + av_qsv_stage *stage = av_mallocz(sizeof(av_qsv_stage)); > > + return stage; > > pointless variable indirection see my latest patch > > > +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. > > > +void av_qsv_add_context_usage(av_qsv_context * qsv, int is_threaded) > > +{ > > + int is_active = 0; > > + > > + is_active = ff_qsv_atomic_inc(&qsv->is_context_active); > > + if (is_active == 1) { > > + memset(&qsv->mfx_session, 0, sizeof(mfxSession)); > > + av_qsv_pipe_list_create(&qsv->pipes, is_threaded); > > + > > + qsv->dts_seq = av_qsv_list_init(is_threaded); > > This init function can fail, but you never check it, same below. see my latest patch > > > +#if HAVE_THREADS > > + if (is_threaded) { > > + qsv->qts_seq_mutex = av_mallocz(sizeof(pthread_mutex_t)); > > + if (qsv->qts_seq_mutex) > > + pthread_mutex_init(qsv->qts_seq_mutex, NULL); > > + > > + } else > > A malloc failure is not a reason to abort or return an error? see my latest patch > > There are more such cases below. > > > +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 > > > + if (end <= start) { > > + new_dts = av_mallocz(sizeof(av_qsv_dts)); > > + if( new_dts ) { > > if (new_dts) > > more similar code below see my latest patch > > > +av_qsv_list *av_qsv_list_init(int is_threaded) > > +{ > > + av_qsv_list *l; > > + > > + l = av_mallocz(sizeof(av_qsv_list)); > > You can merge declaration and initialization. see my latest patch > > > + if (!l) > > + return 0; > > + l->items = av_mallocz(AV_QSV_JOB_SIZE_DEFAULT * sizeof(void *)); > > av_mallocz_array() good point - switched, see my latest patch > > > +#if HAVE_THREADS > > + if (is_threaded) { > > + l->mutex = av_mallocz(sizeof(pthread_mutex_t)); > > + if (l->mutex) > > + pthread_mutex_init(l->mutex, NULL); > > + } else > > +#endif > > + l->mutex = 0; > > + return l; > > +} > > A failure of the second malloc is not a reason to return an error? see my latest patch > > > +int av_qsv_list_add(av_qsv_list * l, void *p) > > +{ > > + int pos = -1; > > + if (!p) { > > + return pos; > > + } > > +#if HAVE_THREADS > > + if (l->mutex) > > + pthread_mutex_lock(l->mutex); > > +#endif > > + if (l->items_count == l->items_alloc) { > > + /* We need a bigger boat */ > > + l->items_alloc += AV_QSV_JOB_SIZE_DEFAULT; > > + l->items = av_realloc(l->items, l->items_alloc * sizeof(void *)); > > + } > > We need a bigger boat? see my latest patch > > > + l->items[l->items_count] = p; > > + pos = (l->items_count); > > + (l->items_count)++; > > pointless () see my latest patch > > > +void av_qsv_list_insert(av_qsv_list * l, int pos, void *p) > > +{ > > + if (!p) > > + return; > > +#if HAVE_THREADS > > + if (l->mutex) > > + pthread_mutex_lock(l->mutex); > > +#endif > > + > > + if (l->items_count == l->items_alloc) { > > + l->items_alloc += AV_QSV_JOB_SIZE_DEFAULT; > > + l->items = av_realloc(l->items, l->items_alloc * sizeof(void *)); > > + } > > unchecked realloc see my latest patch > > > --- /dev/null > > +++ b/libavcodec/qsv.h > > @@ -0,0 +1,469 @@ > > + > > +/** > > + * @defgroup lavc_codec_hwaccel_qsv QSV/MediaSDK based Decode/Encode and > > VPP > > VPP? Video Pre-Processing, see my latest patch > > > + * @ingroup lavc_codec_hwaccel > > + * > > + * As Intel Quick Sync Video (QSV) can decode/preprocess/encode with HW > > + * acceleration. > > HW --> hardware see my latest patch > > 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. > > > +#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) > > space after comma, no space before ( see my latest patch > > > +#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. > > +typedef enum { > > + QSV_PART_ANY = 0, > > + QSV_PART_LOWER, > > + QSV_PART_UPPER > > +} av_qsv_split; > > + > > +typedef struct { > > + int64_t dts; > > +} av_qsv_dts; > > No anonymously typedeffed structs/enums in headers please. see my latest patch > > > +typedef struct av_qsv_config { > > + /** > > + * Set asynch depth of processing with QSV > > async > > > + /** > > + * Distance between I- or P- key frames; if it is zero, the GOP > > structure is unspecified. > > + * Note: If GopRefDist = 1, there are no B-frames used. > > * Note: If GopRefDist = 1 no B-frames are used. > > > + /** > > + * Set amount of additional surfaces might be needed > > .. that might be needed? > > > + * Format: ammount of additional buffers(surfaces+syncs) > > aMount > > > + /** > > + * If pipeline should be sync. > > synchronized? synchronous? like in-sync see my latest patch > > > + /** > > + * if QSV usage is multithreaded. > > + * > > + /** > > + * if QSV use an external allocation (valid per session/mfxSession) > > + * > > Start sentences capitalized. see my latest patch > > > +int av_qsv_get_free_sync(av_qsv_space *, av_qsv_context *); > > +int av_qsv_get_free_surface(av_qsv_space *, av_qsv_context *, mfxFrameInfo > > *, > > + av_qsv_split); > > Add the parameter names. > > > +#endif //AVCODEC_QSV_H > > #endif /* AVCODEC_QSV_H */ > > > --- /dev/null > > +++ b/libavcodec/qsv_h264.c > > @@ -0,0 +1,920 @@ > > + > > + .frame_alloc = { > > + .pthis = &av_qsv_default_system_allocators, > > + .Alloc = ff_qsv_mem_frame_alloc, > > + .Lock = ff_qsv_mem_frame_lock, > > + .Unlock = ff_qsv_mem_frame_unlock, > > + .GetHDL = ff_qsv_mem_frame_getHDL, > > + .Free = ff_qsv_mem_frame_free, > > + }, > > + .buffer_alloc = { > > + .pthis = &av_qsv_default_system_allocators, > > + .Alloc = ff_qsv_mem_buffer_alloc, > > + .Lock = ff_qsv_mem_buffer_lock, > > + .Unlock = ff_qsv_mem_buffer_unlock, > > + .Free = ff_qsv_mem_buffer_free, > > + }, > > Just indent by four spaces, not by random large amounts. > > > +int ff_qsv_nal_find_start_code(uint8_t * pb, size_t size) > > +{ > > + if ((int) size < 4) > > + return 0; > > Why do you cast here? see my latest patch > > > + while ((4 <= size) && ((0 != pb[0]) || (0 != pb[1]) || (1 != pb[2]))) { > > + pb += 1; > > + size -= 1; > > + } > > + if (4 <= size) > > + return 1; > > nit: pb++ and size-- would look more natural IMO > > In general we write "size >= 4", not the other way around. > > > +int ff_qsv_dec_init(AVCodecContext * avctx) > > +{ > > + int ret = 0; > > + mfxStatus sts = MFX_ERR_NONE; > > + size_t current_offset = 6; > > + int header_size = 0; > > + unsigned char *current_position; > > + size_t current_size; > > + > > + av_qsv_context *qsv = avctx->priv_data; > > + av_qsv_space *qsv_decode = qsv->dec_space; > > + av_qsv_config *qsv_config_context = avctx->hwaccel_context; > > nit: vertical align > > > + 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 > > > + { > > + current_position = avctx->extradata; > > + current_size = avctx->extradata_size; > > Why do you need the block braces? just some development overhead left, see my latest patch > > > + if (!ff_qsv_nal_find_start_code(current_position, current_size)) { > > + > > + while (current_offset <= current_size) { > > + int current_nal_size = > > + (unsigned char) current_position[current_offset] << 8 | > > + (unsigned char) current_position[current_offset + 1]; > > + unsigned char nal_type = > > + (unsigned char) current_position[current_offset + 2] & > > 0x1F; > > The casts seem unnecessary. > > > + // fix for PPS as it comes after SPS, so - last > > + if (nal_type == NAL_PPS) { > > + // fix of MFXVideoDECODE_DecodeHeader: needs one > > SLICE to find, any SLICE > > + memcpy(&qsv_decode->p_buf > > + [header_size + current_nal_size], > > Please don't break lines before array indexes. > > > + } else { > > + memcpy(&qsv_decode->p_buf[0], avctx->extradata, > > + avctx->extradata_size); > > + header_size = avctx->extradata_size; > > + memcpy(&qsv_decode->p_buf > > + [header_size], ff_slice_code, sizeof(ff_slice_code)); > > same > > > + sts = MFXVideoDECODE_DecodeHeader(qsv->mfx_session, &qsv_decode->bs, > > + &qsv_decode->m_mfxVideoParam); > > indentation > > > + memset(&qsv_decode->request, 0, sizeof(mfxFrameAllocRequest) * 2); > > + sts = MFXVideoDECODE_QueryIOSurf(qsv->mfx_session, > > + &qsv_decode->m_mfxVideoParam, > > + &qsv_decode->request); > > same > > > + qsv_decode->surface_num = > > + FFMIN(qsv_decode->request[0].NumFrameSuggested + > > + qsv_config_context->async_depth + > > + qsv_config_context->additional_buffers, AV_QSV_SURFACE_NUM); > > same > > > + qsv_decode->request[0].Type = MFX_MEMTYPE_EXTERNAL_FRAME | > > MFX_MEMTYPE_FROM_DECODE; > > + // qsv_decode->request[0].Type |= m_bd3dAlloc ? > > MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET : MFX_MEMTYPE_SYSTEM_MEMORY; > > no commented-out cruft please > > > + 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 > > > + sts = > > + MFXVideoDECODE_Init(qsv->mfx_session, > > + &qsv_decode->m_mfxVideoParam); > > ugly line break, more in other places > > > + av_log_missing_feature( avctx,"Only > > MFX_IOPATTERN_OUT_OPAQUE_MEMORY and MFX_IOPATTERN_OUT_SYSTEM_MEMORY are > > currently supported\n",0); > > no space inside (), space after comma > > The message does not make any sense, it will be > > Only MFX_IOPATTERN_OUT_OPAQUE_MEMORY and MFX_IOPATTERN_OUT_SYSTEM_MEMORY are > currently supported\n > is not implemented. Update your Libav version to the newest one from Git. If > the problem still occurs, it means that your file has a feature which has not > been implemented.\n"); > correct, see my latest patch > > + av_qsv_add_context_usage(qsv, > > + HAVE_THREADS > > + ? (*qsv_config_context)->usage_threaded : > > + HAVE_THREADS); > > indentation > > > +static av_cold int qsv_decode_end(AVCodecContext * avctx) > > +{ > > + mfxStatus sts = MFX_ERR_NONE; > > + av_qsv_context *qsv = avctx->priv_data; > > + av_qsv_config *qsv_config_context = avctx->hwaccel_context; > > align the = > > > + for (int i = 0; i < qsv_decode->surface_num; i++) { > > + av_freep(&qsv_decode->p_surfaces[i]); > > + } > > + qsv_decode->surface_num = 0; > > + > > + for (int i = 0; i < qsv_decode->sync_num; i++) { > > + av_freep(&qsv_decode->p_sync[i]); > > + } > > pointless {} > > > + qsv_decode->sync_num = 0; > > + qsv_decode->is_init_done = 0; > > align > > > +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 > > > + if (current_size) { > > + if (!ff_qsv_nal_find_start_code(avpkt->data, avpkt->size)) > > + while (current_offset <= current_size) { > > + current_nal_size = > > + ((unsigned char) current_position[current_offset - 2] > > + << 24 | (unsigned char) > > + current_position[current_offset - > > + 1] << 16 | (unsigned char) > > + current_position[current_offset] << 8 | (unsigned > > + char) > > + current_position[current_offset + 1]) - 1; > > + nal_type = > > + (unsigned char) current_position[current_offset + 2] & > > 0x1F; > > Do you really need all those unsigned char casts? C++ overhead, see my latest patch > > > + > > + picture->repeat_pict = > > (qsv_decode->m_mfxVideoParam.mfx.FrameInfo.PicStruct & > > MFX_PICSTRUCT_FIELD_REPEATED); > > + picture->top_field_first = > > (qsv_decode->m_mfxVideoParam.mfx.FrameInfo.PicStruct & > > MFX_PICSTRUCT_FIELD_TFF); > > pointless () > > > +// Will be called when seeking > > +static void qsv_flush_dpb(AVCodecContext * avctx) > > +{ > > + av_qsv_context *qsv = avctx->priv_data; > > + av_qsv_space *qsv_decode = qsv->dec_space; > > + > > + qsv_decode->bs.DataOffset = 0; > > + qsv_decode->bs.DataLength = 0; > > + qsv_decode->bs.MaxLength = qsv_decode->p_buf_max_size; > > align the =, more below > > > +mfxStatus ff_qsv_mem_buffer_alloc(mfxHDL pthis, mfxU32 nbytes, mfxU16 type, > > + mfxMemId * mid) > > +{ > > + av_qsv_alloc_buffer *bs; > > + mfxU32 header_size; > > + mfxU8 *buffer_ptr; > > + > > + header_size = AV_QSV_ALIGN32(sizeof(av_qsv_alloc_buffer)); > > + buffer_ptr = (mfxU8 *) av_malloc(header_size + nbytes); > > pointless void* cast > > > --- /dev/null > > +++ b/libavcodec/qsv_h264.h > > @@ -0,0 +1,64 @@ > > + > > +#ifndef AVCODEC_QSV_H264_H > > +#define AVCODEC_QSV_H264_H > > + > > +#include "qsv.h" > > + > > +int ff_qsv_dec_init(AVCodecContext *); > > +int ff_qsv_nal_find_start_code(uint8_t * pb, size_t size); > > + > > +av_cold int ff_qsv_decode_init(AVCodecContext * avctx); > > Drop the av_cold attribute from the header. see my latest patch > Diego > _______________________________________________ > libav-devel mailing list > [email protected] > https://lists.libav.org/mailman/listinfo/libav-devel 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) regards Maxym _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
