On Wed, Jan 30, 2013 at 08:21:48AM +0100, Maxym Dmytrychenko wrote:
> Intel Media SDK, Quick Sync Video/QSV: Initial add of H.264 decoding via
> hwaccel infrastructure
>
> For now: MinGW and MSVC compilers and targeting Windows OS.
Just some comments, not a full review at all ...
Isn't this supposed to be a hwaccel and not a decoder?
> --- a/configure
> +++ b/configure
> @@ -132,6 +132,7 @@ Component options:
> --enable-vaapi enable VAAPI code
> --enable-vda enable VDA code
> --enable-vdpau enable VDPAU code
> + --enable-qsv enable QSV code
Put this in order please.
> @@ -1074,6 +1075,7 @@ CONFIG_LIST="
> vaapi
> vda
> vdpau
> + qsv
> version3
> xmm_clobber_test
> x11grab
same
> @@ -1631,6 +1633,7 @@ wmv3_dxva2_hwaccel_select="vc1_dxva2_hwaccel"
> wmv3_vaapi_hwaccel_select="vc1_vaapi_hwaccel"
> wmv3_vdpau_decoder_select="vc1_vdpau_decoder"
> wmv3_vdpau_hwaccel_select="vc1_vdpau_hwaccel"
> +h264_qsv_decoder_select="qsv h264_decoder"
same
> @@ -3576,6 +3579,13 @@ if ! disabled vdpau && enabled vdpau_vdpau_h; then
>
> +if enabled qsv; then
> + disable qsv
> + check_header msdk/mfxvideo.h && enable qsv
> +else
> + disable qsv
> +fi
This is not how it's done. Have your component declare a _deps entry
in the hwaccel section after line 1600.
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -10,6 +10,7 @@ HEADERS = avcodec.h
> \
> vdpau.h \
> version.h \
> xvmc.h \
> + qsv.h \
order
> @@ -197,6 +198,7 @@ OBJS-$(CONFIG_H264_DECODER) += h264.o
> \
> h264_loopfilter.o h264_direct.o
> \
> cabac.o h264_sei.o h264_ps.o
> \
> h264_refs.o h264_cavlc.o
> h264_cabac.o
> +OBJS-$(CONFIG_H264_QSV_DECODER) += qsv_h264.o qsv.o
> OBJS-$(CONFIG_H264_DXVA2_HWACCEL) += dxva2_h264.o
> OBJS-$(CONFIG_H264_VAAPI_HWACCEL) += vaapi_h264.o
> OBJS-$(CONFIG_H264_VDA_HWACCEL) += vda_h264.o
same
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -151,6 +151,7 @@ void avcodec_register_all(void)
> REGISTER_ENCODER(H263P, h263p);
> REGISTER_DECODER(H264, h264);
> REGISTER_DECODER(H264_VDPAU, h264_vdpau);
> + REGISTER_DECODER(H264_QSV, h264_qsv);
> REGISTER_ENCDEC (HUFFYUV, huffyuv);
> REGISTER_DECODER(IDCIN, idcin);
same
> --- /dev/null
> +++ b/libavcodec/qsv.c
> @@ -0,0 +1,555 @@
> +/* ********************************************************************* *\
> +
> +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.
I bet there's at least a dozen such copyright notices that we need to
reproduce in the documentation by now. What's wrong with the LGPL?
> +int get_free_encode_task(qsv_list * tasks)
*tasks, more below
> +int get_free_sync(qsv_space * space, 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]))
> + && !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 >= REPEAT_NUM) {
> +#endif
> + av_log(NULL, AV_LOG_FATAL, "not enought to have %d sync(s)\n",
enough_
Also, enough of what?
> +int get_free_surface(qsv_space * space, qsv_context * qsv,
> + mfxFrameInfo * info, qsv_split part)
> +{
> + }
> +#if HAVE_THREADS
> + if (++counter >= REPEAT_NUM) {
> +#endif
> + av_log(NULL, AV_LOG_FATAL,
> + "not enought to have %d surface(s)\n", up);
same
> +qsv_stage *qsv_stage_init()
(void)
This should generate a warning, please don't ignore it.
> +{
> + qsv_stage *stage = av_mallocz(sizeof(qsv_stage));
> + return stage;
This looks like a pointless variable indirection to me ..
> +void add_qsv_context_usage(qsv_context * qsv, int is_threaded)
> +{
> + int is_active = 0;
> +
> + is_active = atomic_inc(&qsv->is_context_active);
> + if (is_active == 1) {
> + memset(&qsv->mfx_session, 0, sizeof(mfxSession));
> + qsv_pipe_list_create(&qsv->pipes, is_threaded);
> +
> + qsv->dts_seq = qsv_list_init(is_threaded);
> +
> +#if HAVE_THREADS
> + if (is_threaded) {
> + qsv->qts_seq_mutex = av_mallocz(sizeof(pthread_mutex_t));
> + pthread_mutex_init(qsv->qts_seq_mutex, NULL);
This malloc is unchecked, I have seen more in other places, I'm not
going to point out all since I just skimmed this and probably
overlooked some cases anyway.
> +int qsv_context_clean(qsv_context * qsv)
> +{
> +
> + if (qsv->dts_seq) {
> + while (qsv_list_count(qsv->dts_seq)) {
> + qsv_dts_pop(qsv);
> + }
nit: we usually drop unnecessary {}
> + if (qsv->pipes) {
> + qsv_pipe_list_clean(&qsv->pipes);
> + }
same, more below
> +void qsv_pipe_list_create(qsv_list ** list, int is_threaded)
> +{
> + if (!*list) {
> + *list = qsv_list_init(is_threaded);
> + }
> +}
> +
> +void qsv_pipe_list_clean(qsv_list ** list)
> +{
> + qsv_list *stage;
> + int i = 0;
> + if (*list) {
> + for (i = qsv_list_count(*list); i > 0; i--) {
> + stage = qsv_list_item(*list, i - 1);
> + qsv_flush_stages(*list, &stage);
> + }
> + qsv_list_close(list);
> + }
> +}
I do wonder why you often pass **foo only to dereference it in most
uses inside the functions..
> +// no dublicate of the same value, if end == 0 : working over full length
duPlicate
> --- /dev/null
> +++ b/libavcodec/qsv.h
> @@ -0,0 +1,449 @@
> +
> +#ifndef QSV_H
> +#define QSV_H
Give this an AVCODEC_ prefix
> +#include <stdint.h>
> +#include <string.h>
> +
> +#ifdef HAVE_AV_CONFIG_H
> +#include "config.h"
> +#endif
This conditional inclusion seems to be an autotools reflex; unlearn
it here and drop the ifdef; it's not working as you expect anyway.
> +#if HAVE_THREADS
> +// see get_free_sync, get_free_surface , 100 if usleep(10*1000)(10ms) == 1
> sec
> +#define REPEAT_NUM 100
> +#endif
The define seems harmless enough to skip the ifdefs.
> +typedef struct {
> + int64_t dts;
> +} dts_t;
The _t namespace is reserved for POSIX, don't invade it.
> +static const uint8_t ff_prefix_code[] = { 0x00, 0x00, 0x00, 0x01 };
> +
> +int get_free_sync(qsv_space *, qsv_context *);
> +int get_free_surface(qsv_space *, qsv_context *, mfxFrameInfo *,
> + qsv_split);
> +int get_free_encode_task(qsv_list *);
These functions have rather generic names.
Also, all symbols that are externally visible should have ff_ or av_
prefixes, depending on whether they should be visible only inside or
also outside of the library.
> +#endif //QSV_H
#endif /* AVCODEC_QSV_H */
> --- /dev/null
> +++ b/libavcodec/qsv_h264.c
> @@ -0,0 +1,935 @@
> +
> +static const av_qsv_context default_config = {
> + .qsv_async_depth = 4,
> + .qsv_io_pattern = MFX_IOPATTERN_OUT_OPAQUE_MEMORY,
> + .qsv_additional_buffers = 0,
> + .qsv_sync_need = 0,
> + .qsv_impl_requested = MFX_IMPL_HARDWARE,
> + .qsv_usage_threaded = 0,
> + .qsv_allocators = 0,
vertically align the =
> +static qsv_allocators_space default_system_allocators = {
> + // fill to access mids
> + .space = 0,
> +
> + .frame_alloc = {
> + .pthis = &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 = &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,
> + },
This is oddly indented.
> + } else {
> + if ((*qsv_config_context)->qsv_io_pattern !=
> + MFX_IOPATTERN_OUT_OPAQUE_MEMORY
> + && (*qsv_config_context)->qsv_io_pattern !=
> + MFX_IOPATTERN_OUT_SYSTEM_MEMORY) {
> + av_log(avctx, AV_LOG_FATAL,
> + "Only MFX_IOPATTERN_OUT_OPAQUE_MEMORY and
> MFX_IOPATTERN_OUT_SYSTEM_MEMORY are currently supported, rest is TODO\n");
> + return -1;
av_log_missing_feature()
> + 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);
Please do not break lines at the periods, it looks very odd.
> + // since we dont know it yet from MSDK, let's do just a simple way
> for now
"do not" - that typo brings back memories of some dark past around here ;-p
> + 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;
nit: align the =
You could align more = in many other places...
> +mfxStatus ff_qsv_mem_buffer_alloc(mfxHDL pthis, mfxU32 nbytes, mfxU16 type,
> + mfxMemId * mid)
> +{
> + header_size = QSV_ALIGN32(sizeof(qsv_alloc_buffer));
> + buffer_ptr = (mfxU8 *) av_malloc(header_size + nbytes);
> +
> + if (!buffer_ptr)
> + return MFX_ERR_MEMORY_ALLOC;
Do the MFX_ERR and AVERROR_ values mix without problems?
> +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},
Look at some other file to see how we format this.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
