On Wed, Feb 27, 2013 at 04:49:07PM +0100, Maxym Dmytrychenko wrote:
> "make checkheaders" is fine for MinGW and MSVC
>
> ---
> libavcodec/qsv.c | 549 ++++++++++++++++++++++++++++++++
> libavcodec/qsv.h | 476 ++++++++++++++++++++++++++++
> libavcodec/qsv_h264.c | 835
> +++++++++++++++++++++++++++++++++++++++++++++++++
> libavcodec/qsv_h264.h | 63 ++++
> 4 files changed, 1923 insertions(+), 0 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
Why is this patch separate from 2/2? It makes no sense standalone.
> --- /dev/null
> +++ b/libavcodec/qsv.c
> @@ -0,0 +1,549 @@
> +
> +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;
You can return i directly instead of breaking out of the loop.
> +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++)
Don't declare variables inside for statements like this. Some compilers
choke on it. Supposedly also MSVC, so I wonder why you did not stumble
over the problem. You have a new MSVC version that does not trip up?
> + if (!(*(space->p_sync[i])) &&
> !ff_qsv_is_sync_in_pipe(space->p_sync[i], qsv)) {
Break the long line.
> +int av_qsv_get_free_surface(av_qsv_space *space, av_qsv_context *qsv,
> mfxFrameInfo *info, av_qsv_split part)
long line
> + for (int i = from; i < up; i++)
> + if ((!space->p_surfaces[i]->Data.Locked) &&
> !ff_qsv_is_surface_in_pipe(space->p_surfaces[i], qsv)) {
same
> + av_log(NULL, AV_LOG_FATAL,
> + "Have not enough to have %d surface(s) allocated\n", up);
Have not enough .. what?
> +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;
Please merge the two if-clauses with ||.
> + 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;
> +}
The ret variable is not assigned, so pretty pointless.
> +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;
> +}
ditto
> +av_qsv_stage *av_qsv_stage_init(void)
> +{
> + return (av_qsv_stage *)av_mallocz(sizeof(av_qsv_stage));
> +}
pointless void* cast
> +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);
I don't understand the comment - what is "spaces"?
> +void av_qsv_pipe_list_create(av_qsv_list **list, int is_threaded)
> +{
> + if (!*list)
> + *list = av_qsv_list_init(is_threaded);
> +}
Wouldn't it be simpler to pass list as * instead of ** and dereferencing?
> +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);
> + }
> +}
same
> +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);
> + if (*list)
> + av_qsv_list_add(*list, stage);
> +}
same
> +av_qsv_stage *av_qsv_get_by_mask(av_qsv_list *list, int mask, av_qsv_stage
> **prev, av_qsv_list **this_pipe)
long line
> +{
> + 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++) {
> +}
> +
> +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++) {
You can skip the first initialization of i and a.
> +// 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)
long line
> +{
> + av_qsv_dts *cur_dts = 0;
> + av_qsv_dts *new_dts = 0;
> + int i = 0;
pointless initialization of i
> +av_qsv_list *av_qsv_list_init(int is_threaded)
> +{
> + av_qsv_list *l = av_mallocz(sizeof(av_qsv_list));
> + if (!l)
> + return 0;
> + l->items = av_mallocz_array(AV_QSV_JOB_SIZE_DEFAULT, sizeof(void *));
> + if (!l->items) {
> + av_free(l);
> + 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) {
> + av_free(l->items);
> + av_free(l);
> + return 0;
> + }
> + if (l->mutex)
> + pthread_mutex_init(l->mutex, NULL);
> + } else
> +#endif
> + l->mutex = 0;
> + return l;
> +}
You should return NULL, not 0 for pointers.
> +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) {
> + l->items_alloc += AV_QSV_JOB_SIZE_DEFAULT;
> + l->items = av_realloc(l->items, l->items_alloc * sizeof(void
> *));
> + if (!l->items)
> + return pos;
> + }
> +
> + l->items[l->items_count] = p;
> + pos = (l->items_count);
> + (l->items_count)++;
pointless ()
> +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;
> +}
Avoid names starting with underscores; they are reserved at the file level
or completely. Just "list" would be better here I think.
> +int av_is_qsv_available(mfxIMPL impl, mfxVersion *ver)
> +{
> + mfxStatus sts = MFX_ERR_NONE;
> + mfxSession mfx_session;
> +
> + memset(&mfx_session, 0, sizeof(mfxSession));
Initialize mfx_session to { 0 } instead.
> --- /dev/null
> +++ b/libavcodec/qsv.h
> @@ -0,0 +1,476 @@
> +/*
> + * 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: Remove the empty line.
> +
> +#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
> Video Pre-Processing/VPP
> + * @ingroup lavc_codec_hwaccel
> + *
> + * As Intel Quick Sync Video (QSV) can decode/preprocess/encode with
> hardware
> + * acceleration
> + *
> + * Supported features:
> + * - 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:
> + * - format AV_PIX_FMT_QSV_MPEG2
> + * - format AV_PIX_FMT_QSV_VC1
> + * - IO Pattern:
> + * - VIDEO_MEMORY // MFX_IOPATTERN_OUT_VIDEO_MEMORY
> + * - Allocators:
> + * - Video memory: MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET /
> + * MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET
> + *
> + * 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
> + * 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), if resource_free ||
> usage_threaded option:
> + * 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, if dts_need option:
> + * 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
> +// atomic ops
> +#if defined (__GNUC__)
> +#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>
> +#define ff_qsv_atomic_inc(ptr) InterlockedIncrement(ptr)
> +#define ff_qsv_atomic_dec(ptr) InterlockedDecrement(ptr)
> +#endif
> +// threading implementation
> +#if HAVE_PTHREADS
> +#include <pthread.h>
> +#elif HAVE_W32THREADS
> +#include "w32pthreads.h"
> +#endif
> +#else
> +#define ff_qsv_atomic_inc(ptr) ((*ptr)++)
> +#define ff_qsv_atomic_dec(ptr) ((*ptr)--)
> +#endif
Once again, with feeling: none of this can be in an installed header.
> +// 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
Why should all of this become exported libav API?
> +// 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;
In one case you lowercase the "av" prefix.
Is there a reason not to separate the #defines and the enum?
> +static const uint8_t ff_prefix_code[] = { 0x00, 0x00, 0x00, 0x01 };
A static table in a header is generally not a good idea; it will get
duplicated at each #inclusion.
> +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 **);
Add the parameter names.
> --- /dev/null
> +++ b/libavcodec/qsv_h264.c
> @@ -0,0 +1,835 @@
> +/*
> + * 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: Drop the empty line.
> +int ff_qsv_dec_init(AVCodecContext *avctx)
> +{
> +
> + memset(&qsv->mfx_session, 0, sizeof(mfxSession));
> + qsv->ver.Major = AV_QSV_MSDK_VERSION_MAJOR;
> + qsv->ver.Minor = AV_QSV_MSDK_VERSION_MINOR;
> +
> + AV_QSV_ZERO_MEMORY(qsv_decode->m_mfxVideoParam);
> + AV_QSV_ZERO_MEMORY(qsv_decode->bs);
Sometimes you use this (IMO) pointless macro, sometimes memset ...
> + sts = MFXVideoDECODE_DecodeHeader(qsv->mfx_session, &qsv_decode->bs,
> + &qsv_decode->m_mfxVideoParam);
> + AV_QSV_CHECK_RESULT(sts, MFX_ERR_NONE, sts);
In libav we do not use such macros to check return values generally.
> + qsv_decode->sync_num = FFMIN(qsv_decode->surface_num, AV_QSV_SYNC_NUM);
> + for (int i = 0; i < qsv_decode->sync_num; i++) {
see above
> +int ff_qsv_decode_init(AVCodecContext *avctx)
init functions should be marked as av_cold.
> +{
> + av_qsv_context *qsv;
> + av_qsv_space *qsv_decode;
> + av_qsv_config **qsv_config_context = (av_qsv_config
> **)&avctx->hwaccel_context;
pointless void* cast
> + } else {
> + if ((*qsv_config_context)->io_pattern !=
> MFX_IOPATTERN_OUT_OPAQUE_MEMORY
> + && (*qsv_config_context)->io_pattern !=
> MFX_IOPATTERN_OUT_SYSTEM_MEMORY) {
nit: Move the && to the previous line.
> + av_log_missing_feature(avctx, "MFX_IOPATTERN_OUT_SYSTEM_MEMORY
> type", 0);
This is about to be replaced with avpriv_report_missing_feature, ETA tomorrow.
> + av_log(avctx, AV_LOG_FATAL, "No QSV allocators found for
> clean up\n");
cleanup
> + for (int i = 0; i < qsv_decode->surface_num; i++)
see above
> +static int qsv_decode_frame(AVCodecContext *avctx, void *data,
> + int *data_size, AVPacket *avpkt)
> +{
> + if (current_size) {
> + while (current_offset <= current_size) {
> + current_nal_size =
> + (current_position[current_offset - 2] << 24 |
> + current_position[current_offset - 1] << 16 |
> + current_position[current_offset] << 8 |
> + current_position[current_offset + 1]) - 1;
> + nal_type = 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,
> + ¤t_position[current_offset + 2],
> + current_nal_size + 1);
> + qsv_decode->bs.DataLength += current_nal_size + 1;
> + }
Why the block {}?
> + if (qsv_config_context->dts_need) {
> + //not a drain
> + if ((current_size || qsv_decode->bs.DataLength))
pointless ()
> + while (MFX_ERR_NONE <= sts || MFX_ERR_MORE_SURFACE == sts
> + || MFX_WRN_DEVICE_BUSY == sts) {
nit: Move the || to the previous line.
> + // 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];
I don't understand the comment.
> +mfxStatus ff_qsv_mem_frame_free(mfxHDL pthis,
> + mfxFrameAllocResponse *response)
> +{
> + mfxU32 i;
> +
> + if (response->mids)
> + for (i = 0; i < response->NumFrameActual; i++)
Why isn't i a plain int or unsigned?
> +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))
nit: We generally prefer to write ! in such cases:
if (!(type & MFX_MEMTYPE_SYSTEM_MEMORY))
> +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_DELAY,
> + .flush = qsv_flush_dpb,
> + .long_name = NULL_IF_CONFIG_SMALL("H.264 / AVC / Intel QSV"),
Make this
NULL_IF_CONFIG_SMALL("H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10 (Intel QSV
acceleration)"),
to match the VDPAU one.
> --- /dev/null
> +++ b/libavcodec/qsv_h264.h
> @@ -0,0 +1,63 @@
> +/*
> + * 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: Drop the empty line.
> +int ff_qsv_dec_init(AVCodecContext *);
Please add the parameter name.
> +static 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);
Why are there static declarations in a header?
> +// Default for SYSTEM MEMORY
> +// as from MFXFrameAllocator
> +mfxStatus ff_qsv_mem_frame_alloc(mfxHDL pthis,
> + mfxFrameAllocRequest *request,
> + mfxFrameAllocResponse *response);
I don't understand the comment.
> +#endif //AVCODEC_QSV_H264_H
nit: #endif /* AVCODEC_QSV_H264_H */
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
