Intel Media SDK, Quick Sync Video/QSV: Initial add of H.264 decoder infrastructure

Message ID 1360007375-3346-1-git-send-email-maxym.dmytrychenko@intel.com
State Superseded
Headers show

Commit Message

Maxym Dmytrychenko Feb. 4, 2013, 7:49 p.m.
From: Maxym Dmytrychenko <Maxym.Dmytrychenko@intel.com>

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))
   
---
 Changelog              |    2 +-
 configure              |    9 +
 libavcodec/Makefile    |    2 +
 libavcodec/allcodecs.c |    1 +
 libavcodec/qsv.c       |  556 +++++++++++++++++++++++++++++
 libavcodec/qsv.h       |  469 ++++++++++++++++++++++++
 libavcodec/qsv_h264.c  |  920 ++++++++++++++++++++++++++++++++++++++++++++++++
 libavcodec/qsv_h264.h  |   64 ++++
 libavcodec/version.h   |    2 +-
 libavutil/pixfmt.h     |    1 +
 10 files changed, 2024 insertions(+), 2 deletions(-)
 create mode 100644 libavcodec/qsv.c
 create mode 100644 libavcodec/qsv.h
 create mode 100644 libavcodec/qsv_h264.c
 create mode 100644 libavcodec/qsv_h264.h

Comments

Diego Biurrun Feb. 21, 2013, 3:03 p.m. | #1
On Mon, Feb 04, 2013 at 08:49:35PM +0100, Maxym Dmytrychenko wrote:
> From: Maxym Dmytrychenko <Maxym.Dmytrychenko@intel.com>
> 
> 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
Luca Barbato Feb. 26, 2013, 11:24 a.m. | #2
On 04/02/13 20:49, Maxym Dmytrychenko wrote:
> From: Maxym Dmytrychenko <Maxym.Dmytrychenko@intel.com>

After reading better the code (and compiling and contributing a build
system for the opensource part of qsv) seems to me that it won't fit the
current hwaccel.

I'd refine the decoder and merge it instead of waiting for a better
hwaccel layer or work on the opensource qsv to make it somehow fit.

lu
Maxym Dmytrychenko Feb. 27, 2013, 5:39 p.m. | #3
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 <Maxym.Dmytrychenko@intel.com>
> > 
> > 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
> libav-devel@libav.org
> 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
Diego Biurrun Feb. 27, 2013, 6:50 p.m. | #4
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 <Maxym.Dmytrychenko@intel.com>
> > > 
> > 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.

> > > --- /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.

> > > +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?

> > > --- /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.

> > > +#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.

> > > --- /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.

> > > +        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.

> > > +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 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
Maxym Dmytrychenko Feb. 27, 2013, 10:46 p.m. | #5
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 <Maxym.Dmytrychenko@intel.com>
> > > > 
> > > 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
> libav-devel@libav.org
> 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
Luca Barbato Feb. 28, 2013, 7:47 a.m. | #6
On 27/02/13 23:46, Maxym Dmytrychenko wrote:
> 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

Yes, once the evil plan lands it could be simplified btw.

lu
Diego Biurrun Feb. 28, 2013, 10:38 a.m. | #7
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.

> #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

Does this work with threads disabled?

> #else
> // targeting only for MinGW or MSVC
> #endif

?

> > > > > +#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?

> > > > > --- /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?

> > > > > +        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?

> > > 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.
> 
> any more comments?
> 
> let me to know if new patch should be created.
> I would like to include all feedback(s) into it.

Yes, of course.

Diego
Maxym Dmytrychenko Feb. 28, 2013, 1:47 p.m. | #8
On Thu, Feb 28, 2013 at 11:38 AM, Diego Biurrun <diego@biurrun.de> 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

Again,  will such move help to solve the question?

> #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 ?


Does this work with threads disabled?
>

latest tests were ok,
let me to know if any other results.


> > #else
> > // targeting only for MinGW or MSVC
> > #endif
>
> ?
>
targeting MSVC and MinGW only
if you dont like to - please drop


> > > > > > +#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.

please dont forget - QSV based acceleration offers more that just decode
and it all - has to work together with libav/QSV.

you can have a look on MediaSDK samples and definitions been used there,
files like sample_common\include\sample_defs.h



> > > > > > --- /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?

> > > > > +        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.

there are no drop planned for any of these in the future, from QSV side.


> > > > 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.
> >
> > any more comments?
> >
> > let me to know if new patch should be created.
> > I would like to include all feedback(s) into it.
>
> Yes, of course.
>
> Diego
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>

please see above

regards
Maxym
Diego Biurrun Feb. 28, 2013, 2:16 p.m. | #9
On Thu, Feb 28, 2013 at 02:47:39PM +0100, Max Dm wrote:
> On Thu, Feb 28, 2013 at 11:38 AM, Diego Biurrun <diego@biurrun.de> 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
Maxym Dmytrychenko Feb. 28, 2013, 4:53 p.m. | #10
On Thu, Feb 28, 2013 at 3:16 PM, Diego Biurrun <diego@biurrun.de> 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 <diego@biurrun.de>
> 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
> libav-devel@libav.org
> 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
Diego Biurrun Feb. 28, 2013, 5:27 p.m. | #11
On Thu, Feb 28, 2013 at 05:53:49PM +0100, Max Dm wrote:
> On Thu, Feb 28, 2013 at 3:16 PM, Diego Biurrun <diego@biurrun.de> 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 <diego@biurrun.de>
> > 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?

Probably yes.

> > > 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.

I cannot be bothered to be honest.  There is no git/whatever tree directly
visible online?

> 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.

Let's add the extra parts when we have a need for them, not earlier.
We have had a long history of features added for future use and then
left to rot.

Diego
Maxym Dmytrychenko Feb. 28, 2013, 5:54 p.m. | #12
On Thu, Feb 28, 2013 at 6:27 PM, Diego Biurrun <diego@biurrun.de> wrote:

> On Thu, Feb 28, 2013 at 05:53:49PM +0100, Max Dm wrote:
> > On Thu, Feb 28, 2013 at 3:16 PM, Diego Biurrun <diego@biurrun.de> 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 <diego@biurrun.de>
> > > 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?
>
> Probably yes.
>
> > > > 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.
>
> I cannot be bothered to be honest.  There is no git/whatever tree directly
> visible online?
>
> there is only one way to download MediaSDK,
you can see it at provided link,

> 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.
>
> Let's add the extra parts when we have a need for them, not earlier.
> We have had a long history of features added for future use and then
> left to rot.
>
>
this patch is shared under BSD license,
feel free to adjust it as for libav projects requirements.


> Diego
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>

Patch

diff --git a/Changelog b/Changelog
index c235727..90c81d6 100644
--- 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
diff --git a/configure b/configure
index ebeea83..12b7117 100755
--- 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
@@ -1600,6 +1602,7 @@  zmbv_decoder_select="zlib"
 zmbv_encoder_select="zlib"
 
 # hardware accelerators
+qsv_deps="msdk_mfxvideo_h"
 vaapi_deps="va_va_h"
 vda_deps="VideoDecodeAcceleration_VDADecoder_h pthreads"
 vdpau_deps="vdpau_vdpau_h vdpau_vdpau_x11_h"
@@ -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"
@@ -3578,6 +3582,11 @@  if ! disabled vdpau && enabled vdpau_vdpau_h; then
         { echolog "Please upgrade to libvdpau >= 0.2 if you would like vdpau support." && disable vdpau; }
 fi
 
+# check for MSDK headers
+if ! disabled qsv && check_header msdk/mfxvideo.h; then
+    enable qsv
+fi 
+
 enabled debug && add_cflags -g"$debuglevel" && add_asflags -g"$debuglevel"
 
 # add some useful compiler flags if supported
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 774ea57..b796ff0 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -5,6 +5,7 @@  HEADERS = avcodec.h                                                     \
           avfft.h                                                       \
           dxva2.h                                                       \
           old_codec_ids.h                                               \
+          qsv.h                                                         \
           vaapi.h                                                       \
           vda.h                                                         \
           vdpau.h                                                       \
@@ -306,6 +307,7 @@  OBJS-$(CONFIG_QCELP_DECODER)           += qcelpdec.o                     \
 OBJS-$(CONFIG_QDM2_DECODER)            += qdm2.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
 OBJS-$(CONFIG_R10K_DECODER)            += r210dec.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index 8bfa603..a4f8643 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -150,6 +150,7 @@  void avcodec_register_all(void)
     REGISTER_DECODER(H263I,             h263i);
     REGISTER_ENCODER(H263P,             h263p);
     REGISTER_DECODER(H264,              h264);
+    REGISTER_DECODER(H264_QSV,          h264_qsv);
     REGISTER_DECODER(H264_VDPAU,        h264_vdpau);
     REGISTER_ENCDEC (HUFFYUV,           huffyuv);
     REGISTER_DECODER(IDCIN,             idcin);
diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c
new file mode 100644
index 0000000..43d56d1
--- /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.
+
+\* ********************************************************************* */
+
+#include "qsv.h"
+
+#include "avcodec.h"
+#include "internal.h"
+
+int av_qsv_get_free_encode_task(av_qsv_list * tasks)
+{
+    int ret = MFX_ERR_NOT_FOUND;
+    int i = 0;
+    if (tasks)
+        for (i = 0; i < av_qsv_list_count(tasks); i++) {
+            av_qsv_task *task = av_qsv_list_item(tasks, i);
+            if (task->stage && task->stage->out.p_sync)
+                if (!(*task->stage->out.p_sync)) {
+                    ret = i;
+                    break;
+                }
+        }
+    return ret;
+}
+
+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)) {
+                if (i > space->sync_num_max_used)
+                    space->sync_num_max_used = i;
+                return i;
+            }
+        }
+#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);
+            break;
+#if HAVE_THREADS
+        }
+        av_qsv_sleep(10);
+#endif
+    }
+    return ret;
+}
+
+int av_qsv_get_free_surface(av_qsv_space * space, av_qsv_context * qsv,
+                     mfxFrameInfo * info, av_qsv_split part)
+{
+    int ret = -1;
+    int from = 0;
+    int up = space->surface_num;
+    int counter = 0;
+
+    while (1) {
+        from = 0;
+        up = space->surface_num;
+        if (part == QSV_PART_LOWER)
+            up /= 2;
+        if (part == QSV_PART_UPPER)
+            from = up / 2;
+
+        for (int i = from; i < up; i++) {
+            if ((0 == space->p_surfaces[i]->Data.Locked)
+                && !ff_qsv_is_surface_in_pipe(space->p_surfaces[i], qsv)) {
+                memcpy(&(space->p_surfaces[i]->Info), info,
+                       sizeof(mfxFrameInfo));
+                if (i > space->surface_num_max_used)
+                    space->surface_num_max_used = i;
+                return i;
+            }
+        }
+#if HAVE_THREADS
+        if (++counter >= AV_QSV_REPEAT_NUM_DEFAULT) {
+#endif
+            av_log(NULL, AV_LOG_FATAL,
+                   "not enough to have %d surface(s) allocated\n", up);
+            break;
+#if HAVE_THREADS
+        }
+        av_qsv_sleep(10);
+#endif
+    }
+    return ret;
+}
+
+int ff_qsv_is_surface_in_pipe(mfxFrameSurface1 * p_surface, av_qsv_context * qsv)
+{
+    int ret = 0;
+    int a, b;
+    av_qsv_list *list = 0;
+    av_qsv_stage *stage = 0;
+
+    if (!p_surface)
+        return ret;
+    if (!qsv->pipes)
+        return ret;
+
+    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 (p_surface == stage->out.p_surface ||
+                p_surface == stage->in.p_surface) {
+                return 1;
+            }
+        }
+    }
+    return ret;
+}
+
+int ff_qsv_is_sync_in_pipe(mfxSyncPoint * sync, av_qsv_context * qsv)
+{
+    int ret = 0;
+    int a, b;
+    av_qsv_list *list = 0;
+    av_qsv_stage *stage = 0;
+
+    if (!sync)
+        return ret;
+    if (!qsv->pipes)
+        return ret;
+
+    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;
+            }
+        }
+    }
+    return ret;
+}
+
+av_qsv_stage *av_qsv_stage_init(void)
+{
+    av_qsv_stage *stage = av_mallocz(sizeof(av_qsv_stage));
+    return stage;
+}
+
+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);
+}
+
+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);
+
+#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
+#endif
+            qsv->qts_seq_mutex = 0;
+    }
+}
+
+int av_qsv_context_clean(av_qsv_context * qsv)
+{
+    int is_active = 0;
+    mfxStatus sts = MFX_ERR_NONE;
+
+    is_active = ff_qsv_atomic_dec(&qsv->is_context_active);
+
+    // spaces would have to be cleaned on the own,
+    // here we care about the rest, common stuff
+    if (is_active == 0) {
+
+        if (qsv->dts_seq) {
+            while (av_qsv_list_count(qsv->dts_seq))
+                av_qsv_dts_pop(qsv);
+
+            av_qsv_list_close(&qsv->dts_seq);
+        }
+#if HAVE_THREADS
+        if (qsv->qts_seq_mutex) {
+            pthread_mutex_destroy(qsv->qts_seq_mutex);
+#endif
+            qsv->qts_seq_mutex = 0;
+#if HAVE_THREADS
+        }
+#endif
+
+        if (qsv->pipes)
+            av_qsv_pipe_list_clean(&qsv->pipes);
+
+        if (qsv->mfx_session) {
+            sts = MFXClose(qsv->mfx_session);
+            AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+            qsv->mfx_session = 0;
+        }
+    }
+    return 0;
+}
+
+void av_qsv_pipe_list_create(av_qsv_list ** list, int is_threaded)
+{
+    if (!*list)
+        *list = av_qsv_list_init(is_threaded);
+}
+
+void av_qsv_pipe_list_clean(av_qsv_list ** list)
+{
+    av_qsv_list *stage;
+    int i = 0;
+    if (*list) {
+        for (i = av_qsv_list_count(*list); i > 0; i--) {
+            stage = av_qsv_list_item(*list, i - 1);
+            av_qsv_flush_stages(*list, &stage);
+        }
+        av_qsv_list_close(list);
+    }
+}
+
+void av_qsv_add_stagee(av_qsv_list ** list, av_qsv_stage * stage, int is_threaded)
+{
+    if (!*list)
+        *list = av_qsv_list_init(is_threaded);
+    av_qsv_list_add(*list, stage);
+}
+
+av_qsv_stage *av_qsv_get_last_stage(av_qsv_list * list)
+{
+    av_qsv_stage *stage = 0;
+    int size = 0;
+
+#if HAVE_THREADS
+    if (list->mutex)
+        pthread_mutex_lock(list->mutex);
+#endif
+
+    size = av_qsv_list_count(list);
+    if (size > 0)
+        stage = av_qsv_list_item(list, size - 1);
+
+#if HAVE_THREADS
+    if (list->mutex)
+        pthread_mutex_unlock(list->mutex);
+#endif
+
+    return stage;
+}
+
+void av_qsv_flush_stages(av_qsv_list * list, av_qsv_list ** item)
+{
+    int i = 0;
+    av_qsv_stage *stage = 0;
+
+    for (i = 0; i < av_qsv_list_count(*item); i++) {
+        stage = av_qsv_list_item(*item, i);
+        av_qsv_stage_clean(&stage);
+        // should actually remove from the list but ok...
+    }
+    av_qsv_list_rem(list, *item);
+    av_qsv_list_close(item);
+}
+
+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;
+}
+
+av_qsv_list *av_qsv_pipe_by_stage(av_qsv_list * list, av_qsv_stage * stage)
+{
+    av_qsv_list *item = 0;
+    av_qsv_stage *cur_stage = 0;
+    int i = 0;
+    int a = 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++) {
+            cur_stage = av_qsv_list_item(item, a);
+            if (cur_stage == stage)
+                return item;
+        }
+    }
+    return 0;
+}
+
+// no duplicate of the same value, if end == 0 : working over full length
+void av_qsv_dts_ordered_insert(av_qsv_context * qsv, int start, int end,
+                            int64_t dts, int iter)
+{
+    av_qsv_dts *cur_dts = 0;
+    av_qsv_dts *new_dts = 0;
+    int i = 0;
+
+#if HAVE_THREADS
+    if (iter == 0 && qsv->qts_seq_mutex)
+        pthread_mutex_lock(qsv->qts_seq_mutex);
+#endif
+
+    if (end == 0)
+        end = av_qsv_list_count(qsv->dts_seq);
+
+    if (end <= start) {
+        new_dts = av_mallocz(sizeof(av_qsv_dts));
+        if( new_dts ) {
+            new_dts->dts = dts;
+            av_qsv_list_add(qsv->dts_seq, new_dts);
+        }
+    } else
+        for (i = end; i > start; i--) {
+            cur_dts = av_qsv_list_item(qsv->dts_seq, i - 1);
+            if (cur_dts->dts < dts) {
+                new_dts = av_mallocz(sizeof(av_qsv_dts));
+                if( new_dts ) {
+                    new_dts->dts = dts;
+                    av_qsv_list_insert(qsv->dts_seq, i, new_dts);
+                }
+                break;
+            } else if (cur_dts->dts == dts)
+                break;
+        }
+#if HAVE_THREADS
+    if (iter == 0 && qsv->qts_seq_mutex)
+        pthread_mutex_unlock(qsv->qts_seq_mutex);
+#endif
+}
+
+void av_qsv_dts_pop(av_qsv_context * qsv)
+{
+    av_qsv_dts *item = 0;
+
+#if HAVE_THREADS
+    if (qsv && qsv->qts_seq_mutex)
+        pthread_mutex_lock(qsv->qts_seq_mutex);
+#endif
+
+    if (av_qsv_list_count(qsv->dts_seq)) {
+        item = av_qsv_list_item(qsv->dts_seq, 0);
+        av_qsv_list_rem(qsv->dts_seq, item);
+        av_free(item);
+    }
+#if HAVE_THREADS
+    if (qsv && qsv->qts_seq_mutex)
+        pthread_mutex_unlock(qsv->qts_seq_mutex);
+#endif
+}
+
+
+av_qsv_list *av_qsv_list_init(int is_threaded)
+{
+    av_qsv_list *l;
+
+    l = av_mallocz(sizeof(av_qsv_list));
+    if (!l)
+        return 0;
+    l->items = av_mallocz(AV_QSV_JOB_SIZE_DEFAULT * sizeof(void *));
+    if (!l->items)
+        return 0;
+    l->items_alloc = AV_QSV_JOB_SIZE_DEFAULT;
+
+#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;
+}
+
+int av_qsv_list_count(av_qsv_list * l)
+{
+    return l->items_count;
+}
+
+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 *));
+    }
+
+    l->items[l->items_count] = p;
+    pos = (l->items_count);
+    (l->items_count)++;
+
+#if HAVE_THREADS
+    if (l->mutex)
+        pthread_mutex_unlock(l->mutex);
+#endif
+
+    return pos;
+}
+
+void av_qsv_list_rem(av_qsv_list * l, void *p)
+{
+    int i;
+
+#if HAVE_THREADS
+    if (l->mutex)
+        pthread_mutex_lock(l->mutex);
+#endif
+
+    /* Find the item in the list */
+    for (i = 0; i < l->items_count; i++) {
+        if (l->items[i] == p) {
+            /* Shift all items after it sizeof( void * ) bytes earlier */
+            memmove(&l->items[i], &l->items[i + 1],
+                    (l->items_count - i - 1) * sizeof(void *));
+
+            (l->items_count)--;
+            break;
+        }
+    }
+
+#if HAVE_THREADS
+    if (l->mutex)
+        pthread_mutex_unlock(l->mutex);
+#endif
+}
+
+void *av_qsv_list_item(av_qsv_list * l, int i)
+{
+    if (i < 0 || i >= l->items_count)
+        return NULL;
+
+    return l->items[i];
+}
+
+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 *));
+    }
+
+    if (l->items_count != pos) {
+        memmove(&l->items[pos + 1], &l->items[pos],
+                (l->items_count - pos) * sizeof(void *));
+    }
+
+    l->items[pos] = p;
+    (l->items_count)++;
+
+#if HAVE_THREADS
+    if (l->mutex)
+        pthread_mutex_unlock(l->mutex);
+#endif
+}
+
+void av_qsv_list_close(av_qsv_list ** _l)
+{
+    av_qsv_list *l = *_l;
+
+#if HAVE_THREADS
+    if (l->mutex)
+        pthread_mutex_destroy(&l->mutex);
+#endif
+
+    av_free(l->items);
+    av_free(l);
+
+    *_l = NULL;
+}
+
+int av_is_qsv_available(mfxIMPL impl, mfxVersion * ver)
+{
+    mfxStatus sts = MFX_ERR_NONE;
+    mfxSession mfx_session;
+
+    memset(&mfx_session, 0, sizeof(mfxSession));
+    sts = MFXInit(impl, ver, &mfx_session);
+    if (sts >= 0)
+        MFXClose(mfx_session);
+    return sts;
+}
diff --git a/libavcodec/qsv.h b/libavcodec/qsv.h
new file mode 100644
index 0000000..d5ea9b7
--- /dev/null
+++ b/libavcodec/qsv.h
@@ -0,0 +1,469 @@ 
+/* ********************************************************************* *\
+
+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.
+
+\* ********************************************************************* */
+
+#ifndef AVCODEC_QSV_H
+#define AVCODEC_QSV_H
+
+/**
+ * @file
+ * @ingroup lavc_codec_hwaccel_qsv
+ * Common header for QSV/MediaSDK acceleration
+ */
+
+/**
+ * @defgroup lavc_codec_hwaccel_qsv QSV/MediaSDK based Decode/Encode and VPP
+ * @ingroup lavc_codec_hwaccel
+ *
+ *  As Intel Quick Sync Video (QSV) can decode/preprocess/encode with HW
+ *  acceleration.
+ *
+ *  Supported features:
+ *    - access:
+ *      - format AV_PIX_FMT_QSV_H264, AVCodec decoder based implementation
+ *      - name "h264_qsv", avcodec_find_decoder_by_name( "h264_qsv")
+ *    - IO Pattern:
+ *      - Opaque memory: MFX_IOPATTERN_OUT_OPAQUE_MEMORY // Video memory is
+ *                       MFX_IMPL_HARDWARE or MFX_IMPL_AUTO and runtime support,
+ *                       otherwise: System Memory
+ *      - System memory: MFX_IOPATTERN_OUT_SYSTEM_MEMORY
+ *    - Allocators:
+ *      - default allocator for System memory: MFX_MEMTYPE_SYSTEM_MEMORY
+ *    - details:
+ *      implementation as "per frame"
+ *
+ *  TODO list:
+ *    - access:
+ *      - format AV_PIX_FMT_QSV_MPEG2
+ *      - format AV_PIX_FMT_QSV_VC1
+ *      - format AV_PIX_FMT_QSV, see "details" below
+ *    - IO Pattern:
+ *      - VIDEO_MEMORY  // MFX_IOPATTERN_OUT_VIDEO_MEMORY
+ *    - Allocators:
+ *      - Video memory: MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET /
+ *                      MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET
+ *    - details:
+ *      "per slice" support: AV_PIX_FMT_QSV with AVHWAccel based implementation
+ *
+ *  Note av_qsv_config struct required to fill in via
+ *  AVCodecContext.hwaccel_context
+ *
+ *  As per frame, note AVFrame.data[2] (qsv_atom) used for frame atom id,
+ *  data/linesize should be used together with SYSTEM_MEMORY and tested
+ *
+ *  Note: Compilation would require:
+ *   - Intel MediaSDK headers, Full SDK is avaialble from the original web site:
+ *                     http://software.intel.com/en-us/vcsource/tools/media-SDK
+ *     Will be referenced as msdk/*.h (mfxdefs.h, mfxstructures.h, ... )
+ *  and
+ *  - Final application has to link against Intel MediaSDK dispatcher, available
+ *     at MediaSDK as well
+ *
+ *  Target OS: as per available dispatcher and driver support
+ *
+ *  Implementation details:
+ *   Provided struct av_qsv_context contain several struct av_qsv_space(s) for decode,
+ *   VPP and encode.
+ *   av_qsv_space just contain needed environment for the appropriate action.
+ *   Based on this - pipeline (see pipes) will be build to pass details such as
+ *   mfxFrameSurface1* and mfxSyncPoint* from one action to the next.
+ *
+ *  Resources re-usage (av_qsv_flush_stages):
+ *     av_qsv_context *qsv = (av_qsv_context *)video_codec_ctx->priv_data;
+ *     av_qsv_list *pipe = (av_qsv_list *)video_frame->data[2];
+ *     av_qsv_flush_stages( qsv->pipes, &pipe );
+ *
+ *  DTS re-usage:
+ *     av_qsv_dts_pop(qsv);
+ *
+ *   for video,DX9/11 memory it has to be Unlock'ed as well
+ *
+ *  Implementation is thread aware and uses synchronization point(s) from MediaSDK
+ *  as per configuration.
+ *
+ *  For the details of MediaSDK usage and options available - please refer to the
+ *  available documentation at MediaSDK.
+ *
+ *  Feature set used from MSDK is defined by AV_QSV_MSDK_VERSION_MAJOR and
+ *  AV_QSV_MSDK_VERSION_MINOR
+ *
+ * @{
+ */
+
+#include <stdint.h>
+#include <string.h>
+
+#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
+
+#include "msdk/mfxvideo.h"
+#include "libavutil/mem.h"
+#include "libavutil/time.h"
+
+// sleep is defined in milliseconds
+#define av_qsv_sleep(x) av_usleep((x)*1000)
+
+#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
+
+typedef enum AV_QSV_STAGE_TYPE {
+
+#define AV_QSV_DECODE_MASK   0x001
+    AV_QSV_DECODE   = 0x001,
+
+#define AV_QSV_VPP_MASK      0x0F0
+    // "Mandatory VPP filter" , might be with "Hint-based VPP filters"
+    AV_QSV_VPP_DEFAULT = 0x010,
+    // "User Modules" etc
+    AV_QSV_VPP_USER = 0x020,
+
+#define av_QSV_ENCODE_MASK   0x100
+    AV_QSV_ENCODE   = 0x100
+#define AV_QSV_ANY_MASK      0xFFF
+} AV_QSV_STAGE_TYPE;
+
+typedef struct av_qsv_stage {
+    AV_QSV_STAGE_TYPE type;
+    struct {
+        mfxBitstream *p_bs;
+        mfxFrameSurface1 *p_surface;
+    } in;
+    struct {
+        mfxBitstream *p_bs;
+        mfxFrameSurface1 *p_surface;
+        mfxSyncPoint *p_sync;
+    } out;
+} av_qsv_stage;
+
+typedef struct av_qsv_task {
+    mfxBitstream *bs;
+    av_qsv_stage *stage;
+} av_qsv_task;
+
+typedef struct av_qsv_list {
+    // practically pthread_mutex_t
+    void *mutex;
+
+    void **items;
+    int items_alloc;
+
+    int items_count;
+} av_qsv_list;
+
+typedef struct av_qsv_space {
+
+    uint8_t is_init_done;
+
+    AV_QSV_STAGE_TYPE type;
+
+    mfxVideoParam m_mfxVideoParam;
+
+    mfxFrameAllocResponse response;
+    mfxFrameAllocRequest request[2];    // [0] - in, [1] - out, if needed
+
+    mfxExtOpaqueSurfaceAlloc ext_opaque_alloc;
+    mfxExtBuffer *p_ext_params;
+
+    uint16_t surface_num_max_used;
+    uint16_t surface_num;
+    mfxFrameSurface1 *p_surfaces[AV_QSV_SURFACE_NUM];
+
+    uint16_t sync_num_max_used;
+    uint16_t sync_num;
+    mfxSyncPoint *p_sync[AV_QSV_SYNC_NUM];
+
+    mfxBitstream bs;
+    uint8_t *p_buf;
+    size_t p_buf_max_size;
+
+    // only for encode and tasks
+    av_qsv_list *tasks;
+
+    // storage for allocations/mfxMemId*
+    mfxMemId *mids;
+} av_qsv_space;
+
+typedef struct av_qsv_context {
+    volatile int is_context_active;
+
+    mfxIMPL impl;
+    mfxSession mfx_session;
+    mfxVersion ver;
+
+    // decode
+    av_qsv_space *dec_space;
+    // encode
+    av_qsv_space *enc_space;
+    // vpp
+    av_qsv_list *vpp_space;
+
+    av_qsv_list *pipes;
+
+    // MediaSDK starting from API version 1.6 includes DecodeTimeStamp
+    // in addition to TimeStamp
+    // see also AV_QSV_MSDK_VERSION_MINOR , AV_QSV_MSDK_VERSION_MAJOR
+    av_qsv_list *dts_seq;
+
+    // practically pthread_mutex_t
+    void *qts_seq_mutex;
+
+} av_qsv_context;
+
+typedef enum {
+    QSV_PART_ANY = 0,
+    QSV_PART_LOWER,
+    QSV_PART_UPPER
+} av_qsv_split;
+
+typedef struct {
+    int64_t dts;
+} av_qsv_dts;
+
+typedef struct av_qsv_alloc_frame {
+    mfxU32 id;
+    mfxFrameInfo info;
+} av_qsv_alloc_frame;
+
+typedef struct av_qsv_alloc_buffer {
+    mfxU32 id;
+    mfxU32 nbytes;
+    mfxU16 type;
+} av_qsv_alloc_buffer;
+
+typedef struct av_qsv_allocators_space {
+    av_qsv_space *space;
+    mfxFrameAllocator frame_alloc;
+    mfxBufferAllocator buffer_alloc;
+} av_qsv_allocators_space;
+
+typedef struct av_qsv_config {
+    /**
+     * Set asynch depth of processing with QSV
+     * Format: 0 and more
+     *
+     * - encoding: Set by user.
+     * - decoding: Set by user.
+     */
+    int async_depth;
+
+    /**
+     * Range of numbers that indicate trade-offs between quality and speed.
+     * Format: from 1/MFX_TARGETUSAGE_BEST_QUALITY to 7/MFX_TARGETUSAGE_BEST_SPEED inclusive
+     *
+     * - encoding: Set by user.
+     * - decoding: unused
+     */
+    int target_usage;
+
+    /**
+     * Number of reference frames; if NumRefFrame = 0, this parameter is not specified.
+     * Format: 0 and more
+     *
+     * - encoding: Set by user.
+     * - decoding: unused
+     */
+    int num_ref_frame;
+
+    /**
+     * 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.
+     *
+     * - encoding: Set by user.
+     * - decoding: unused
+     */
+     int gop_ref_dist;
+
+    /**
+     * Number of pictures within the current GOP (Group of Pictures); if GopPicSize=0,
+     * then the GOP size is unspecified. If GopPicSize=1, only I-frames are used.
+     *
+     * - encoding: Set by user.
+     * - decoding: unused
+     */
+     int gop_pic_size;
+
+    /**
+     * Set type of surfaces used with QSV
+     * Format: "IOPattern enum" of Media SDK
+     *
+     * - encoding: Set by user.
+     * - decoding: Set by user.
+     */
+    int io_pattern;
+
+    /**
+     * Set amount of additional surfaces might be needed
+     * Format: ammount of additional buffers(surfaces+syncs)
+     * to allocate in advance
+     *
+     * - encoding: Set by user.
+     * - decoding: Set by user.
+     */
+    int additional_buffers;
+
+    /**
+     * If pipeline should be sync.
+     * Format: wait time in milliseconds,
+     *         AV_QSV_SYNC_TIME_DEFAULT/10000 might be a good value
+     *
+     * - encoding: Set by user.
+     * - decoding: Set by user.
+     */
+    int sync_need;
+
+    /**
+     * Type of implementation needed
+     *
+     * - encoding: Set by user.
+     * - decoding: Set by user.
+     */
+    int impl_requested;
+
+    /**
+     * if QSV usage is multithreaded.
+     * Format: Yes/No, 1/0
+     *
+     * - encoding: Set by user.
+     * - decoding: Set by user.
+     */
+    int usage_threaded;
+
+    /**
+     * if QSV use an external allocation (valid per session/mfxSession)
+     * Format: pointer to allocators, if default: 0
+     *
+     * note that:
+     * System Memory:   can be used without provided and external allocator,
+     *  meaning MediaSDK will use an internal one
+     * Video Memory:    in this case - we must provide an external allocator
+     * Also, Media SDK session doesn't require external allocator if the application
+     *  uses opaque memory
+     *
+     * Calls SetFrameAllocator/SetBufferAllocator
+     * (MFXVideoCORE_SetFrameAllocator/MFXVideoCORE_SetBufferAllocator)
+     * are to pass allocators to Media SDK
+     *
+     * - encoding: Set by user.
+     * - decoding: Set by user.
+     */
+    av_qsv_allocators_space *allocators;
+
+} av_qsv_config;
+
+static const uint8_t ff_prefix_code[] = { 0x00, 0x00, 0x00, 0x01 };
+
+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);
+int av_qsv_get_free_encode_task(av_qsv_list *);
+
+int av_is_qsv_available(mfxIMPL, mfxVersion *);
+
+void av_qsv_add_context_usage(av_qsv_context *, int);
+
+void av_qsv_pipe_list_create(av_qsv_list **, int);
+void av_qsv_pipe_list_clean(av_qsv_list **);
+
+void av_qsv_add_stagee(av_qsv_list **, av_qsv_stage *, int);
+av_qsv_stage *av_qsv_get_last_stage(av_qsv_list *);
+av_qsv_stage *av_qsv_get_by_mask(av_qsv_list *, int, av_qsv_stage **, av_qsv_list **);
+av_qsv_list *av_qsv_pipe_by_stage(av_qsv_list *, av_qsv_stage *);
+void av_qsv_flush_stages(av_qsv_list *, av_qsv_list **);
+
+void av_qsv_dts_ordered_insert(av_qsv_context *, int, int, int64_t, int);
+void av_qsv_dts_pop(av_qsv_context *);
+
+av_qsv_stage *av_qsv_stage_init(void);
+void av_qsv_stage_clean(av_qsv_stage **);
+int av_qsv_context_clean(av_qsv_context *);
+
+int ff_qsv_is_sync_in_pipe(mfxSyncPoint *, av_qsv_context *);
+int ff_qsv_is_surface_in_pipe(mfxFrameSurface1 *, av_qsv_context *);
+
+av_qsv_list *av_qsv_list_init(int);
+int av_qsv_list_count(av_qsv_list *);
+int av_qsv_list_add(av_qsv_list *, void *);
+void av_qsv_list_rem(av_qsv_list *, void *);
+void av_qsv_list_insert(av_qsv_list *, int, void *);
+void *av_qsv_list_item(av_qsv_list *, int);
+void av_qsv_list_close(av_qsv_list **);
+
+/* @} */
+
+#endif                          //AVCODEC_QSV_H
diff --git a/libavcodec/qsv_h264.c b/libavcodec/qsv_h264.c
new file mode 100644
index 0000000..97b611f
--- /dev/null
+++ b/libavcodec/qsv_h264.c
@@ -0,0 +1,920 @@ 
+/* ********************************************************************* *\
+
+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.
+
+\* ********************************************************************* */
+
+#include "h264.h"
+#include "h264data.h"
+#include "qsv_h264.h"
+
+static av_qsv_config av_qsv_default_config = {
+    .async_depth        = AV_QSV_ASYNC_DEPTH_DEFAULT,
+    .target_usage       = MFX_TARGETUSAGE_BALANCED,
+    .num_ref_frame      = 0,
+    .gop_ref_dist       = 0,
+    .gop_pic_size       = 0,
+    .io_pattern         = MFX_IOPATTERN_OUT_OPAQUE_MEMORY,
+    .additional_buffers = 0,
+    .sync_need          = 0,
+    .impl_requested     = MFX_IMPL_HARDWARE,
+    .usage_threaded     = 0,
+    .allocators         = 0,
+};
+
+static av_qsv_allocators_space av_qsv_default_system_allocators = {
+    // fill to access mids
+    .space = 0,
+
+    .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,
+                     },
+};
+
+static const uint8_t ff_slice_code[] = { 0x00, 0x00, 0x01, 0x65 };
+
+int ff_qsv_nal_find_start_code(uint8_t * pb, size_t size)
+{
+    if ((int) size < 4)
+        return 0;
+
+    while ((4 <= size) && ((0 != pb[0]) || (0 != pb[1]) || (1 != pb[2]))) {
+        pb += 1;
+        size -= 1;
+    }
+
+    if (4 <= size)
+        return 1;
+
+    return 0;
+}
+
+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;
+
+    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);
+    {
+        current_position    = avctx->extradata;
+        current_size        = avctx->extradata_size;
+
+        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;
+
+                if (nal_type == NAL_SPS || nal_type == NAL_PPS) {
+                    memcpy(&qsv_decode->p_buf[header_size], ff_prefix_code,
+                           sizeof(ff_prefix_code));
+                    header_size += sizeof(ff_prefix_code);
+                    memcpy(&qsv_decode->p_buf[header_size],
+                           &current_position[current_offset + 2],
+                           current_nal_size);
+
+                    // 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],
+                               ff_slice_code, current_nal_size);
+                        header_size += sizeof(ff_slice_code);
+                    }
+                }
+
+                header_size += current_nal_size;
+                current_offset += current_nal_size + 3;
+            }
+        } 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));
+            header_size += sizeof(ff_slice_code);
+        }
+    }
+
+    qsv_decode->bs.Data         = qsv_decode->p_buf;
+    qsv_decode->bs.DataLength   = header_size;
+    qsv_decode->bs.MaxLength    = qsv_decode->p_buf_max_size;
+
+    if (qsv_decode->bs.DataLength > qsv_decode->bs.MaxLength) {
+        av_log(avctx, AV_LOG_FATAL, "DataLength > MaxLength\n");
+        return -1;
+    }
+
+    sts = MFXVideoDECODE_DecodeHeader(qsv->mfx_session, &qsv_decode->bs,
+                                    &qsv_decode->m_mfxVideoParam);
+    AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+
+    qsv_decode->bs.DataLength   -= sizeof(ff_slice_code);
+    qsv_decode->bs.DataFlag     = MFX_BITSTREAM_COMPLETE_FRAME;
+
+    memset(&qsv_decode->request, 0, sizeof(mfxFrameAllocRequest) * 2);
+    sts = MFXVideoDECODE_QueryIOSurf(qsv->mfx_session,
+                                   &qsv_decode->m_mfxVideoParam,
+                                   &qsv_decode->request);
+    AV_QSV_IGNORE_MFX_STS(sts, MFX_WRN_PARTIAL_ACCELERATION);
+    AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+
+    qsv_decode->surface_num =
+        FFMIN(qsv_decode->request[0].NumFrameSuggested +
+            qsv_config_context->async_depth +
+            qsv_config_context->additional_buffers, AV_QSV_SURFACE_NUM);
+    if (qsv_decode->surface_num <= 0)
+        qsv_decode->surface_num = AV_QSV_SURFACE_NUM;
+
+    if (qsv_decode->m_mfxVideoParam.IOPattern ==
+        MFX_IOPATTERN_OUT_SYSTEM_MEMORY) {
+
+        // as per non-opaque memory:
+        if (!qsv_config_context->allocators) {
+            av_log(avctx, AV_LOG_INFO,
+                   "Using default allocators for QSV decode\n");
+            ((av_qsv_config *) avctx->hwaccel_context)->allocators =
+                &av_qsv_default_system_allocators;
+        }
+
+        qsv_config_context->allocators->space = qsv_decode;
+
+        qsv_decode->request[0].NumFrameMin       = qsv_decode->surface_num;
+        qsv_decode->request[0].NumFrameSuggested = qsv_decode->surface_num;
+
+        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;
+        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));
+        memcpy(&(qsv_decode->p_surfaces[i]->Info),
+               &(qsv_decode->request[0].Info), sizeof(mfxFrameInfo));
+
+        // for an external(like DX9/11) based allocation:
+        // we bind:
+        //    m_pmfxSurfaces[i].Data.MemId = m_mfxResponse.mids[i];
+        // else, System memory:
+        if (qsv_decode->m_mfxVideoParam.IOPattern ==
+            MFX_IOPATTERN_OUT_SYSTEM_MEMORY) {
+            sts =
+                qsv_config_context->allocators->
+                frame_alloc.Lock(qsv_config_context->allocators,
+                                 qsv_decode->response.mids[i],
+                                 &(qsv_decode->p_surfaces[i]->Data));
+            AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+        }
+    }
+
+    qsv_decode->sync_num = FFMIN(qsv_decode->surface_num, AV_QSV_SYNC_NUM);
+    for (int i = 0; i < qsv_decode->sync_num; i++) {
+        qsv_decode->p_sync[i] = av_mallocz(sizeof(mfxSyncPoint));
+        AV_QSV_CHECK_POINTER(qsv_decode->p_sync[i], AVERROR(ENOMEM));
+    }
+
+    memset(&qsv_decode->ext_opaque_alloc, 0,
+           sizeof(mfxExtOpaqueSurfaceAlloc));
+
+    if (qsv_decode->m_mfxVideoParam.IOPattern ==
+        MFX_IOPATTERN_OUT_OPAQUE_MEMORY) {
+        qsv_decode->ext_opaque_alloc.Header.BufferId = MFX_EXTBUFF_OPAQUE_SURFACE_ALLOCATION;
+        qsv_decode->ext_opaque_alloc.Header.BufferSz = sizeof(mfxExtOpaqueSurfaceAlloc);
+        qsv_decode->p_ext_params = (mfxExtBuffer *) & qsv_decode->ext_opaque_alloc;
+
+        qsv_decode->ext_opaque_alloc.Out.Surfaces   = qsv_decode->p_surfaces;
+        qsv_decode->ext_opaque_alloc.Out.NumSurface = qsv_decode->surface_num;
+        qsv_decode->ext_opaque_alloc.Out.Type       = qsv_decode->request[0].Type;
+
+        qsv_decode->m_mfxVideoParam.ExtParam    = &qsv_decode->p_ext_params;
+        qsv_decode->m_mfxVideoParam.NumExtParam = 1;
+    }
+
+    sts =
+        MFXVideoDECODE_Init(qsv->mfx_session,
+                            &qsv_decode->m_mfxVideoParam);
+    AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+
+    qsv_decode->is_init_done = 1;
+    return ret;
+}
+
+av_cold int ff_qsv_decode_init(AVCodecContext * avctx)
+{
+    av_qsv_context *qsv;
+    av_qsv_space *qsv_decode;
+    av_qsv_config **qsv_config_context =
+        (av_qsv_config **) & avctx->hwaccel_context;
+
+    qsv = av_mallocz(sizeof(av_qsv_context));
+    if (!qsv)
+        return AVERROR(ENOMEM);
+    avctx->priv_data = qsv;
+
+    qsv_decode = av_mallocz(sizeof(av_qsv_space));
+    if (!qsv_decode)
+        return AVERROR(ENOMEM);
+    qsv->dec_space = qsv_decode;
+
+
+    if (qsv_decode->is_init_done)
+        return 0;
+
+    qsv_decode->p_buf_max_size = AV_QSV_BUF_SIZE_DEFAULT;
+    qsv_decode->p_buf = av_malloc(qsv_decode->p_buf_max_size * sizeof(uint8_t));
+    if (!qsv_decode->p_buf)
+        return AVERROR(ENOMEM);
+
+    if (!(*qsv_config_context)) {
+        av_log(avctx, AV_LOG_INFO,
+               "Using default config for QSV decode\n");
+        avctx->hwaccel_context = &av_qsv_default_config;
+    } else {
+        if ((*qsv_config_context)->io_pattern !=
+            MFX_IOPATTERN_OUT_OPAQUE_MEMORY
+            && (*qsv_config_context)->io_pattern !=
+            MFX_IOPATTERN_OUT_SYSTEM_MEMORY) {
+            av_log_missing_feature( avctx,"Only MFX_IOPATTERN_OUT_OPAQUE_MEMORY and MFX_IOPATTERN_OUT_SYSTEM_MEMORY are currently supported\n",0);
+            return AVERROR_PATCHWELCOME;
+        }
+    }
+
+    av_qsv_add_context_usage(qsv,
+                          HAVE_THREADS
+                          ? (*qsv_config_context)->usage_threaded :
+                          HAVE_THREADS);
+
+    // allocation of p_sync and p_surfaces inside of ff_qsv_dec_init
+    return ff_qsv_dec_init(avctx);
+}
+
+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;
+
+    if (qsv) {
+        av_qsv_space *qsv_decode = qsv->dec_space;
+        if (qsv_decode && qsv_decode->is_init_done) {
+            // todo: change to AV_LOG_INFO
+            av_log(avctx, AV_LOG_QUIET,
+                   "qsv_decode report done, max_surfaces: %u/%u , max_syncs: %u/%u\n",
+                   qsv_decode->surface_num_max_used,
+                   qsv_decode->surface_num, qsv_decode->sync_num_max_used,
+                   qsv_decode->sync_num);
+        }
+
+        if (qsv_config_context
+            && qsv_config_context->io_pattern ==
+            MFX_IOPATTERN_OUT_SYSTEM_MEMORY) {
+            if (qsv_config_context->allocators) {
+                sts =
+                    qsv_config_context->allocators->
+                    frame_alloc.Free(qsv_config_context->allocators,
+                                     &qsv_decode->response);
+                AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+            } else {
+                av_log(avctx, AV_LOG_FATAL,
+                       "No QSV allocators found for clean up\n");
+            }
+        }
+        // closing the own resources
+        av_freep(&qsv_decode->p_buf);
+
+        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]);
+        }
+        qsv_decode->sync_num = 0;
+        qsv_decode->is_init_done = 0;
+
+        av_freep(&qsv->dec_space);
+
+        // closing commong stuff
+        av_qsv_context_clean(qsv);
+    }
+
+    return 0;
+}
+
+static int qsv_decode_frame(AVCodecContext * avctx, void *data,
+                            int *data_size, AVPacket * avpkt)
+{
+    mfxStatus sts = MFX_ERR_NONE;
+    av_qsv_context *qsv = avctx->priv_data;
+    av_qsv_space *qsv_decode = qsv->dec_space;
+    av_qsv_config *qsv_config_context = avctx->hwaccel_context;
+    int *got_picture_ptr = data_size;
+    int ret_value = 1;
+    uint8_t *current_position = avpkt->data;
+    int current_size = avpkt->size;
+    int frame_processed = 0;
+    size_t frame_length = 0;
+    int surface_idx = 0;
+
+    int sync_idx = 0;
+    int current_nal_size;
+    unsigned char nal_type;
+    av_qsv_stage *new_stage = 0;
+    mfxBitstream *input_bs = NULL;
+    size_t current_offset = 2;
+    av_qsv_list *qsv_atom = 0;
+    av_qsv_list *pipe = 0;
+
+    AVFrame *picture = (AVFrame *) data;
+
+    *got_picture_ptr = 0;
+
+    if (qsv_decode->bs.DataOffset + qsv_decode->bs.DataLength +
+        current_size > qsv_decode->bs.MaxLength) {
+        memmove(&qsv_decode->bs.Data[0],
+                qsv_decode->bs.Data + qsv_decode->bs.DataOffset,
+                qsv_decode->bs.DataLength);
+        qsv_decode->bs.DataOffset = 0;
+    }
+
+    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;
+                {
+                    frame_length += current_nal_size;
+
+                    memcpy(&qsv_decode->bs.Data[0] +
+                           qsv_decode->bs.DataLength +
+                           qsv_decode->bs.DataOffset, ff_prefix_code,
+                           sizeof(ff_prefix_code));
+                    qsv_decode->bs.DataLength += sizeof(ff_prefix_code);
+                    memcpy(&qsv_decode->bs.Data[0] +
+                           qsv_decode->bs.DataLength +
+                           qsv_decode->bs.DataOffset,
+                           &current_position[current_offset + 2],
+                           current_nal_size + 1);
+                    qsv_decode->bs.DataLength += current_nal_size + 1;
+                }
+                current_offset += current_nal_size + 5;
+        } else {
+            memcpy(&qsv_decode->bs.Data[0] +
+                   qsv_decode->bs.DataLength +
+                   qsv_decode->bs.DataOffset, avpkt->data, avpkt->size);
+            qsv_decode->bs.DataLength += avpkt->size;
+        }
+
+        if (qsv_decode->bs.DataLength > qsv_decode->bs.MaxLength) {
+            av_log(avctx, AV_LOG_FATAL, "DataLength > MaxLength\n");
+            return -1;
+        }
+    }
+
+    if (frame_length || current_size == 0) {
+
+        qsv_decode->bs.TimeStamp = avpkt->pts;
+
+        //not a drain
+        if ((current_size || qsv_decode->bs.DataLength))
+            av_qsv_dts_ordered_insert(qsv, 0, 0, qsv_decode->bs.TimeStamp, 0);
+
+        sts = MFX_ERR_NONE;
+        // ignore warnings, where warnings >0 , and not error codes <0
+        while (MFX_ERR_NONE <= sts || MFX_ERR_MORE_SURFACE == sts
+               || MFX_WRN_DEVICE_BUSY == sts) {
+            if (MFX_ERR_MORE_SURFACE == sts || MFX_ERR_NONE == sts) {
+                surface_idx =
+                    av_qsv_get_free_surface(qsv_decode, qsv,
+                                     &qsv_decode->request[0].Info,
+                                     QSV_PART_ANY);
+                if (surface_idx == -1) {
+                    *got_picture_ptr = 0;
+                    return 0;
+                }
+            }
+
+            if (MFX_WRN_DEVICE_BUSY == sts)
+                av_qsv_sleep(10);
+
+            sync_idx = av_qsv_get_free_sync(qsv_decode, qsv);
+            if (sync_idx == -1) {
+                *got_picture_ptr = 0;
+                return 0;
+            }
+            new_stage = av_qsv_stage_init();
+
+            input_bs = NULL;
+            // if to drain last ones
+            if (current_size || qsv_decode->bs.DataLength)
+                input_bs = &qsv_decode->bs;
+
+            // Decode a frame asynchronously (returns immediately)
+            // very first IDR / SLICE should be with SPS/PPS
+            sts =
+                MFXVideoDECODE_DecodeFrameAsync(qsv->mfx_session, input_bs,
+                                                qsv_decode->p_surfaces
+                                                [surface_idx],
+                                                &new_stage->out.p_surface,
+                                                qsv_decode->p_sync
+                                                [sync_idx]);
+
+            // have some results
+            if (MFX_ERR_NONE <= sts && MFX_WRN_DEVICE_BUSY != sts &&
+                MFX_WRN_VIDEO_PARAM_CHANGED != sts) {
+
+                new_stage->type         = AV_QSV_DECODE;
+                new_stage->in.p_bs      = input_bs;
+                new_stage->in.p_surface = qsv_decode->p_surfaces[surface_idx];
+
+                // see MFXVideoDECODE_DecodeFrameAsync , will be filled from there, if output
+                new_stage->out.p_sync = qsv_decode->p_sync[sync_idx];
+                pipe =
+                    av_qsv_list_init(HAVE_THREADS ?
+                                  qsv_config_context->usage_threaded :
+                                  HAVE_THREADS);
+                av_qsv_add_stagee(&pipe, new_stage,
+                              HAVE_THREADS ?
+                              qsv_config_context->usage_threaded :
+                              HAVE_THREADS);
+
+                av_qsv_list_add(qsv->pipes, pipe);
+                qsv_atom = pipe;
+
+                // usage for forced decode sync and results, can be avoided if sync done by next stage
+                // also note wait time for Sync and possible usage with MFX_WRN_IN_EXECUTION check
+                if (qsv_config_context->sync_need) {
+                    sts =
+                        MFXVideoCORE_SyncOperation(qsv->mfx_session,
+                                                   *new_stage->out.p_sync,
+                                                   qsv_config_context->
+                                                   sync_need);
+                    AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+
+                    // no need to wait more -> force off
+                    *new_stage->out.p_sync  = 0;
+                    new_stage->out.p_sync   = 0;
+                }
+
+                sts = MFX_ERR_NONE;
+                break;
+            }
+            av_qsv_stage_clean(&new_stage);
+
+            /*
+               Can be because of:
+               - runtime situation:
+               - drain procedure:
+               At the end of the bitstream, the application continuously calls the MFXVideoDECODE_DecodeFrameAsync function with a
+               NULL bitstream pointer to drain any remaining frames cached within the Intel
+               Media SDK decoder, until the function returns MFX_ERR_MORE_DATA.
+             */
+            if (MFX_ERR_MORE_DATA == sts) {
+                // not a drain
+                if (current_size) {
+                    *got_picture_ptr = 0;
+                    return avpkt->size;
+                }
+                // drain
+                break;
+            }
+
+            if (MFX_ERR_MORE_SURFACE == sts || MFX_ERR_MORE_SURFACE == sts)
+                continue;
+
+            AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
+        }
+
+        frame_processed = 1;
+    }
+
+    if (frame_processed) {
+
+        if (current_size) {
+            *got_picture_ptr    = 1;
+            ret_value           = avpkt->size;
+        } else {
+            if (MFX_ERR_MORE_DATA != sts) {
+                *got_picture_ptr    = 1;
+                ret_value           = avpkt->size;
+            } else {
+                *got_picture_ptr = 0;
+                return 0;
+            }
+        }
+
+        picture->pkt_pts            = avpkt->pts;
+        picture->pts                = avpkt->pts;
+
+        picture->repeat_pict        = (qsv_decode->m_mfxVideoParam.mfx.FrameInfo.PicStruct & MFX_PICSTRUCT_FIELD_REPEATED);
+        picture->interlaced_frame   = !(qsv_decode->m_mfxVideoParam.mfx.FrameInfo.PicStruct & MFX_PICSTRUCT_PROGRESSIVE);
+        picture->top_field_first    = (qsv_decode->m_mfxVideoParam.mfx.FrameInfo.PicStruct & MFX_PICSTRUCT_FIELD_TFF);
+
+        // since we do not know it yet from MSDK, let's do just a simple way for now
+        picture->key_frame          = (avctx->frame_number == 0) ? 1 : 0;
+
+        if (qsv_decode->m_mfxVideoParam.IOPattern ==
+            MFX_IOPATTERN_OUT_SYSTEM_MEMORY) {
+            picture->data[0]        = new_stage->out.p_surface->Data.Y;
+            picture->data[1]        = new_stage->out.p_surface->Data.VU;
+            picture->linesize[0]    = new_stage->out.p_surface->Info.Width;
+            picture->linesize[1]    = new_stage->out.p_surface->Info.Width;
+        } else {
+            picture->data[0]        = 0;
+            picture->data[1]        = 0;
+            picture->linesize[0]    = 0;
+            picture->linesize[1]    = 0;
+        }
+
+        picture->data[2]            = qsv_atom;
+        picture->linesize[2]        = 0;
+    }
+    return ret_value;
+}
+
+// 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;
+}
+
+
+mfxStatus ff_qsv_mem_frame_alloc(mfxHDL pthis,
+                                 mfxFrameAllocRequest * request,
+                                 mfxFrameAllocResponse * response)
+{
+    mfxStatus sts = MFX_ERR_NONE;
+
+    mfxU32 numAllocated = 0;
+
+    mfxU32 width = AV_QSV_ALIGN32(request->Info.Width);
+    mfxU32 height = AV_QSV_ALIGN32(request->Info.Height);
+    mfxU32 nbytes;
+
+    av_qsv_allocators_space *this_alloc = (av_qsv_allocators_space *) pthis;
+    av_qsv_alloc_frame *fs;
+
+    if (!this_alloc->space)
+        return MFX_ERR_NOT_INITIALIZED;
+
+    switch (request->Info.FourCC) {
+    case MFX_FOURCC_YV12:
+    case MFX_FOURCC_NV12:
+        nbytes =
+            width * height + (width >> 1) * (height >> 1) +
+            (width >> 1) * (height >> 1);
+        break;
+    case MFX_FOURCC_RGB3:
+        nbytes = width * height + width * height + width * height;
+        break;
+    case MFX_FOURCC_RGB4:
+        nbytes =
+            width * height + width * height + width * height +
+            width * height;
+        break;
+    case MFX_FOURCC_YUY2:
+        nbytes =
+            width * height + (width >> 1) * (height) +
+            (width >> 1) * (height);
+        break;
+    default:
+        return MFX_ERR_UNSUPPORTED;
+    }
+
+    this_alloc->space->mids =
+        av_malloc(sizeof(mfxMemId) * request->NumFrameSuggested);
+    if (!this_alloc->space->mids)
+        return MFX_ERR_MEMORY_ALLOC;
+
+    // allocate frames
+    for (numAllocated = 0; numAllocated < request->NumFrameSuggested;
+         numAllocated++) {
+        sts =
+            this_alloc->buffer_alloc.Alloc(this_alloc->buffer_alloc.pthis,
+                                           nbytes +
+                                           AV_QSV_ALIGN32(sizeof
+                                                       (av_qsv_alloc_frame)),
+                                           request->Type,
+                                           &(this_alloc->
+                                             space->mids[numAllocated]));
+
+        if (MFX_ERR_NONE != sts)
+            break;
+
+        sts =
+            this_alloc->buffer_alloc.Lock(this_alloc->buffer_alloc.pthis,
+                                          this_alloc->
+                                          space->mids[numAllocated],
+                                          (mfxU8 **) & fs);
+
+        if (MFX_ERR_NONE != sts)
+            break;
+
+        fs->id = AV_QSV_ID_FRAME;
+        fs->info = request->Info;
+        this_alloc->buffer_alloc.Unlock(this_alloc->buffer_alloc.pthis,
+                                        this_alloc->
+                                        space->mids[numAllocated]);
+    }
+
+    // check the number of allocated frames
+    if (numAllocated < request->NumFrameMin)
+        return MFX_ERR_MEMORY_ALLOC;
+
+    response->NumFrameActual = (mfxU16) numAllocated;
+    response->mids = this_alloc->space->mids;
+
+    return MFX_ERR_NONE;
+}
+
+mfxStatus ff_qsv_mem_frame_lock(mfxHDL pthis, mfxMemId mid,
+                                mfxFrameData * ptr)
+{
+    mfxStatus sts = MFX_ERR_NONE;
+    av_qsv_alloc_frame *fs = 0;
+    mfxU16 width;
+    mfxU16 height;
+
+    av_qsv_allocators_space *this_alloc = (av_qsv_allocators_space *) pthis;
+
+    if (!this_alloc->space)
+        return MFX_ERR_NOT_INITIALIZED;
+    if (!ptr)
+        return MFX_ERR_NULL_PTR;
+
+
+    sts =
+        this_alloc->buffer_alloc.Lock(this_alloc->buffer_alloc.pthis, mid,
+                                      (mfxU8 **) & fs);
+
+    if (MFX_ERR_NONE != sts)
+        return sts;
+
+    if (AV_QSV_ID_FRAME != fs->id) {
+        this_alloc->buffer_alloc.Unlock(this_alloc->buffer_alloc.pthis,
+                                        mid);
+        return MFX_ERR_INVALID_HANDLE;
+    }
+
+    width = (mfxU16) AV_QSV_ALIGN32(fs->info.Width);
+    height = (mfxU16) AV_QSV_ALIGN32(fs->info.Height);
+    ptr->B = ptr->Y =
+        (mfxU8 *) fs + AV_QSV_ALIGN32(sizeof(av_qsv_allocators_space));
+
+    switch (fs->info.FourCC) {
+    case MFX_FOURCC_NV12:
+        ptr->U = ptr->Y + width * height;
+        ptr->V = ptr->U + 1;
+        ptr->Pitch = width;
+        break;
+    case MFX_FOURCC_YV12:
+        ptr->V = ptr->Y + width * height;
+        ptr->U = ptr->V + (width >> 1) * (height >> 1);
+        ptr->Pitch = width;
+        break;
+    case MFX_FOURCC_YUY2:
+        ptr->U = ptr->Y + 1;
+        ptr->V = ptr->Y + 3;
+        ptr->Pitch = 2 * width;
+        break;
+    case MFX_FOURCC_RGB3:
+        ptr->G = ptr->B + 1;
+        ptr->R = ptr->B + 2;
+        ptr->Pitch = 3 * width;
+        break;
+    case MFX_FOURCC_RGB4:
+        ptr->G = ptr->B + 1;
+        ptr->R = ptr->B + 2;
+        ptr->A = ptr->B + 3;
+        ptr->Pitch = 4 * width;
+        break;
+    default:
+        return MFX_ERR_UNSUPPORTED;
+    }
+
+    return MFX_ERR_NONE;
+}
+
+mfxStatus ff_qsv_mem_frame_unlock(mfxHDL pthis, mfxMemId mid,
+                                  mfxFrameData * ptr)
+{
+    mfxStatus sts = MFX_ERR_NONE;
+    av_qsv_allocators_space *this_alloc = (av_qsv_allocators_space *) pthis;
+
+    sts =
+        this_alloc->buffer_alloc.Unlock(this_alloc->buffer_alloc.pthis,
+                                        mid);
+
+    if (MFX_ERR_NONE != sts)
+        return sts;
+
+    if (NULL != ptr) {
+        ptr->Pitch = 0;
+        ptr->Y = 0;
+        ptr->U = 0;
+        ptr->V = 0;
+    }
+
+    return MFX_ERR_NONE;
+}
+
+mfxStatus ff_qsv_mem_frame_getHDL(mfxHDL pthis, mfxMemId mid,
+                                  mfxHDL * handle)
+{
+    return MFX_ERR_UNSUPPORTED;
+}
+
+mfxStatus ff_qsv_mem_frame_free(mfxHDL pthis,
+                                mfxFrameAllocResponse * response)
+{
+    mfxStatus sts = MFX_ERR_NONE;
+    av_qsv_allocators_space *this_alloc = (av_qsv_allocators_space *) pthis;
+    mfxU32 i;
+
+    if (!response)
+        return MFX_ERR_NULL_PTR;
+
+    if (!this_alloc->space)
+        return MFX_ERR_NOT_INITIALIZED;
+
+    if (response->mids)
+        for (i = 0; i < response->NumFrameActual; i++) {
+            if (response->mids[i]) {
+                sts =
+                    this_alloc->buffer_alloc.Free(this_alloc->
+                                                  buffer_alloc.pthis,
+                                                  response->mids[i]);
+                if (MFX_ERR_NONE != sts)
+                    return sts;
+            }
+        }
+
+    av_freep(&response->mids);
+
+    return sts;
+}
+
+
+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;
+
+    if (!mid)
+        return MFX_ERR_NULL_PTR;
+
+    if (0 == (type & MFX_MEMTYPE_SYSTEM_MEMORY))
+        return MFX_ERR_UNSUPPORTED;
+
+    header_size = AV_QSV_ALIGN32(sizeof(av_qsv_alloc_buffer));
+    buffer_ptr = (mfxU8 *) av_malloc(header_size + nbytes);
+
+    if (!buffer_ptr)
+        return MFX_ERR_MEMORY_ALLOC;
+
+    bs = (av_qsv_alloc_buffer *) buffer_ptr;
+    bs->id = AV_QSV_ID_BUFFER;
+    bs->type = type;
+    bs->nbytes = nbytes;
+    *mid = (mfxHDL) bs;
+    return MFX_ERR_NONE;
+}
+
+mfxStatus ff_qsv_mem_buffer_lock(mfxHDL pthis, mfxMemId mid, mfxU8 ** ptr)
+{
+    av_qsv_alloc_buffer *bs;
+
+    if (!ptr)
+        return MFX_ERR_NULL_PTR;
+
+    bs = (av_qsv_alloc_buffer *) mid;
+
+    if (!bs)
+        return MFX_ERR_INVALID_HANDLE;
+    if (AV_QSV_ID_BUFFER != bs->id)
+        return MFX_ERR_INVALID_HANDLE;
+
+    *ptr = (mfxU8 *) bs + AV_QSV_ALIGN32(sizeof(av_qsv_alloc_buffer));
+    return MFX_ERR_NONE;
+}
+
+mfxStatus ff_qsv_mem_buffer_unlock(mfxHDL pthis, mfxMemId mid)
+{
+    av_qsv_alloc_buffer *bs = (av_qsv_alloc_buffer *) mid;
+
+    if (!bs || AV_QSV_ID_BUFFER != bs->id)
+        return MFX_ERR_INVALID_HANDLE;
+
+    return MFX_ERR_NONE;
+}
+
+mfxStatus ff_qsv_mem_buffer_free(mfxHDL pthis, mfxMemId mid)
+{
+    av_qsv_alloc_buffer *bs = (av_qsv_alloc_buffer *) mid;
+    if (!bs || AV_QSV_ID_BUFFER != bs->id)
+        return MFX_ERR_INVALID_HANDLE;
+
+    av_freep(&bs);
+    return MFX_ERR_NONE;
+}
+
+
+AVCodec ff_h264_qsv_decoder = {
+    .name           = "h264_qsv",
+    .type           = AVMEDIA_TYPE_VIDEO,
+    .id             = AV_CODEC_ID_H264,
+    .init           = ff_qsv_decode_init,
+    .close          = qsv_decode_end,
+    .decode         = qsv_decode_frame,
+    .capabilities   = CODEC_CAP_DR1 | CODEC_CAP_DELAY,
+    .flush          = qsv_flush_dpb,
+    .long_name      = NULL_IF_CONFIG_SMALL("H.264 / AVC / Intel QSV"),
+    .pix_fmts       = (const enum PixelFormat[]) {AV_PIX_FMT_QSV_H264,
+                                                  AV_PIX_FMT_NONE},
+};
diff --git a/libavcodec/qsv_h264.h b/libavcodec/qsv_h264.h
new file mode 100644
index 0000000..a59f3f6
--- /dev/null
+++ b/libavcodec/qsv_h264.h
@@ -0,0 +1,64 @@ 
+/* ********************************************************************* *\
+
+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.
+
+\* ********************************************************************* */
+
+#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);
+static av_cold int qsv_decode_end(AVCodecContext * avctx);
+static int qsv_decode_frame(AVCodecContext * avctx, void *data,
+                            int *data_size, AVPacket * avpkt);
+static void qsv_flush_dpb(AVCodecContext * avctx);
+
+
+// Default for SYSTEM MEMORY
+// as from MFXFrameAllocator
+mfxStatus ff_qsv_mem_frame_alloc(mfxHDL pthis,
+                                 mfxFrameAllocRequest * request,
+                                 mfxFrameAllocResponse * response);
+mfxStatus ff_qsv_mem_frame_lock(mfxHDL pthis, mfxMemId mid,
+                                mfxFrameData * ptr);
+mfxStatus ff_qsv_mem_frame_unlock(mfxHDL pthis, mfxMemId mid,
+                                  mfxFrameData * ptr);
+mfxStatus ff_qsv_mem_frame_getHDL(mfxHDL pthis, mfxMemId mid,
+                                  mfxHDL * handle);
+mfxStatus ff_qsv_mem_frame_free(mfxHDL pthis,
+                                mfxFrameAllocResponse * response);
+// as from mfxBufferAllocator
+mfxStatus ff_qsv_mem_buffer_alloc(mfxHDL pthis, mfxU32 nbytes, mfxU16 type,
+                                  mfxMemId * mid);
+mfxStatus ff_qsv_mem_buffer_lock(mfxHDL pthis, mfxMemId mid, mfxU8 ** ptr);
+mfxStatus ff_qsv_mem_buffer_unlock(mfxHDL pthis, mfxMemId mid);
+mfxStatus ff_qsv_mem_buffer_free(mfxHDL pthis, mfxMemId mid);
+
+#endif                          //AVCODEC_QSV_H264_H
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 80da6e2..7594760 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -27,7 +27,7 @@ 
  */
 
 #define LIBAVCODEC_VERSION_MAJOR 54
-#define LIBAVCODEC_VERSION_MINOR 41
+#define LIBAVCODEC_VERSION_MINOR 42
 #define LIBAVCODEC_VERSION_MICRO  0
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
index 1863099..c6448f8 100644
--- a/libavutil/pixfmt.h
+++ b/libavutil/pixfmt.h
@@ -179,6 +179,7 @@  enum AVPixelFormat {
     AV_PIX_FMT_YUVA444P16BE, ///< planar YUV 4:4:4 64bpp, (1 Cr & Cb sample per 1x1 Y & A samples, big-endian)
     AV_PIX_FMT_YUVA444P16LE, ///< planar YUV 4:4:4 64bpp, (1 Cr & Cb sample per 1x1 Y & A samples, little-endian)
     AV_PIX_FMT_VDPAU,     ///< HW acceleration through VDPAU, Picture.data[3] contains a VdpVideoSurface
+    AV_PIX_FMT_QSV_H264,  ///< H.264 HW decoding with QSV, data[2] contains qsv_atom information for MFX_IOPATTERN_OUT_OPAQUE_MEMORY, MFX_IOPATTERN_OUT_VIDEO_MEMORY
     AV_PIX_FMT_NB,        ///< number of pixel formats, DO NOT USE THIS if you want to link with shared libav* because the number of formats might differ between versions
 
 #if FF_API_PIX_FMT