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.
Does your patch pass "make check"? I suspect at least the checkheaders
target will not pass.
> --- 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.
> --- 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.
> @@ -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.
> @@ -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.
> --- 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.
> --- /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.
> +#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.
> +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.
> +#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.
> +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.
> + 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.
> + av_log(NULL, AV_LOG_FATAL,
> + "not enough to have %d surface(s) allocated\n", up);
no verb again
> + 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 {}
> +av_qsv_stage *av_qsv_stage_init(void)
> +{
> + av_qsv_stage *stage = av_mallocz(sizeof(av_qsv_stage));
> + return stage;
pointless variable indirection
> +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.
> +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.
> +#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?
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?
> + if (end <= start) {
> + new_dts = av_mallocz(sizeof(av_qsv_dts));
> + if( new_dts ) {
if (new_dts)
more similar code below
> +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.
> + if (!l)
> + return 0;
> + l->items = av_mallocz(AV_QSV_JOB_SIZE_DEFAULT * sizeof(void *));
av_mallocz_array()
> +#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?
> +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?
> + l->items[l->items_count] = p;
> + pos = (l->items_count);
> + (l->items_count)++;
pointless ()
> +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
> --- /dev/null
> +++ b/libavcodec/qsv.h
> @@ -0,0 +1,469 @@
> +
> +/**
> + * @defgroup lavc_codec_hwaccel_qsv QSV/MediaSDK based Decode/Encode and VPP
VPP?
> + * @ingroup lavc_codec_hwaccel
> + *
> + * As Intel Quick Sync Video (QSV) can decode/preprocess/encode with HW
> + * acceleration.
HW --> hardware
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.
> +#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 (
> +#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.
> +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.
> +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?
> + /**
> + * if QSV usage is multithreaded.
> + *
> + /**
> + * if QSV use an external allocation (valid per session/mfxSession)
> + *
Start sentences capitalized.
> +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?
> + 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?
> + {
> + current_position = avctx->extradata;
> + current_size = avctx->extradata_size;
Why do you need the block braces?
> + 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.
> + 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");
> + 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
> + 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?
> +
> + 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.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
