[FFmpeg-devel] [PATCH] CrystalHD decoder support v4

[Benoit Fouet]

Hi,

mostly minor remarks. But I think you should now wait for experts to
comment before bothering sending another patch.

On Sun, 06 Feb 2011 16:56:36 -0700 Philip Langdale wrote:
> diff --git a/libavcodec/crystalhd.c b/libavcodec/crystalhd.c
> new file mode 100644
> index 0000000..b17cb81
> --- /dev/null
> +++ b/libavcodec/crystalhd.c
> @@ -0,0 +1,852 @@
> +/*****************************************************************************
> + * Helper functions
> + ****************************************************************************/
> +
> +static inline uint8_t id2subtype(CHDContext *priv, enum CodecID id)
> +{

This should be BC_MEDIA_SUBTYPE, not uint8_t

> +/*
> + * The OpaqueList is built in decode order, while elements will be removed
> + * in presentation order. If frames are reordered, this means we must be
> + * able to remove elements that are not the first element.
> + */
> +static uint64_t opaque_list_pop(CHDContext *priv, uint64_t fake_timestamp)
> +{
> +    OpaqueList *node = priv->head;
> +

OK, this could be done with "OpaqueList **node = &priv->head" to avoid
the special case; not sure this is really worth it though.

> +static av_cold int init(AVCodecContext *avctx)
> +{
> +    CHDContext* priv;
> +    BC_STATUS ret;
> +    BC_INPUT_FORMAT format = {
> +        .FGTEnable   = FALSE,
> +        .Progressive = TRUE,
> +        .OptFlags    = 0x80000000 | vdecFrameRate59_94 | 0x40,
> +        .width       = avctx->width,
> +        .height      = avctx->height,
> +    };
> +
> +    uint8_t subtype;
> +

BC_MEDIA_SUBTYPE? Maybe even use format.mSubtype directly?

> +    case BC_MSUBTYPE_AVC1:
> +        {
> +            uint8_t *dummy_p;
> +            int dummy_int;
> +            AVBitStreamFilterContext *bsfc;
> +
> +            uint32_t orig_data_size = avctx->extradata_size;
> +            uint8_t *orig_data = av_malloc(orig_data_size);
> +            if (!orig_data) {
> +                av_log(avctx, AV_LOG_ERROR,
> +                       "Failed to allocate copy of extradata\n");
> +                return AVERROR(ENOMEM);
> +            }
> +            memcpy(orig_data, avctx->extradata, orig_data_size);
> +
> +
> +            bsfc= av_bitstream_filter_init("h264_mp4toannexb");
> +            if (!bsfc) {
> +                av_log(avctx, AV_LOG_ERROR,
> +                       "Cannot open the h264_mp4toannexb BSF!\n");

missing "av_free(orig_data);"

> +                return AVERROR_BSF_NOT_FOUND;
> +            }
> +            av_bitstream_filter_filter(bsfc, avctx, NULL, &dummy_p,
> +                                       &dummy_int, NULL, 0, 0);

you should check the error code here.

> +            av_bitstream_filter_close(bsfc);
> +
> +            priv->sps_pps_buf     = avctx->extradata;
> +            priv->sps_pps_size    = avctx->extradata_size;
> +            avctx->extradata      = orig_data;
> +            avctx->extradata_size = orig_data_size;
> +
> +            format.pMetaData   = priv->sps_pps_buf;
> +            format.metaDataSz  = priv->sps_pps_size;
> +            format.startCodeSz = (avctx->extradata[4] & 0x03) + 1;
> +        }
> +        break;

> +    case BC_MSUBTYPE_H264:
> +        format.startCodeSz = 4;
> +        // Fall-through
> +    case BC_MSUBTYPE_VC1:
> +    case BC_MSUBTYPE_WVC1:
> +    case BC_MSUBTYPE_WMV3:
> +    case BC_MSUBTYPE_WMVA:
> +    case BC_MSUBTYPE_MPEG1VIDEO:
> +    case BC_MSUBTYPE_MPEG2VIDEO:
> +    case BC_MSUBTYPE_DIVX:
> +    case BC_MSUBTYPE_DIVX311:
> +        format.pMetaData  = avctx->extradata;
> +        format.metaDataSz = avctx->extradata_size;
> +        break;
> +    default:
> +        av_log(avctx, AV_LOG_ERROR, "CrystalHD: Unknown codec name\n");
> +        return -1;

EINVAL?

> +    }
> +    format.mSubtype = subtype;
> +
> +    /* Get a decoder instance */
> +    av_log(avctx, AV_LOG_VERBOSE, "CrystalHD: starting up\n");
> +    // Initialize the Link and Decoder devices
> +    ret = DtsDeviceOpen(&priv->dev, mode);
> +    if (ret != BC_STS_SUCCESS) {
> +        av_log(avctx, AV_LOG_VERBOSE, "CrystalHD: DtsDeviceOpen failed\n");
> +        uninit(avctx);
> +        return -1;

maybe an error label with this uninit+return pattern could be added.
also, this -1 should be changed to a "real" error code.

> +static inline CopyRet copy_frame(AVCodecContext *avctx,
> +                                 BC_DTS_PROC_OUT *output,
> +                                 void *data, int *data_size,
> +                                 uint8_t second_field)
> +{
> +    BC_STATUS ret;
> +    BC_DTS_STATUS decoder_status;
> +    uint8_t next_frame_same;
> +    uint8_t interlaced;
> +    uint8_t need_second_field;
> +
> +    CHDContext *priv = avctx->priv_data;
> +
> +    uint8_t bottom_field = (output->PicInfo.flags & VDEC_FLAG_BOTTOMFIELD) ==
> +                           VDEC_FLAG_BOTTOMFIELD;
> +    uint8_t bottom_first = !!(output->PicInfo.flags & VDEC_FLAG_BOTTOM_FIRST);
> +
> +    int width    = output->PicInfo.width * 2; // 16bits per pixel for YUYV
> +    int height   = output->PicInfo.height;
> +    uint8_t *src = output->Ybuff;
> +    uint8_t *dst;
> +    int dStride;
> +
> +    ret = DtsGetDriverStatus(priv->dev, &decoder_status);
> +    if (ret != BC_STS_SUCCESS) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "CrystalHD: GetDriverStatus failed: %u\n", ret);
> +       return RET_ERROR;
> +    }
> +
> +    next_frame_same   = output->PicInfo.picture_number ==
> +                        (decoder_status.picNumFlags & ~0x40000000);
> +    interlaced        = ((output->PicInfo.flags & VDEC_FLAG_INTERLACED_SRC) &&
> +                         !(output->PicInfo.flags & VDEC_FLAG_UNKNOWN_SRC)) ||
> +                        next_frame_same || bottom_field || second_field;
> +    need_second_field = interlaced && (!bottom_field == !bottom_first);
> +

as bottom_field and bottom_first are boolean now, the second part can
be simplified to bottom_field == bottom_first.

> +static inline CopyRet receive_frame(AVCodecContext *avctx,
> +                                    void *data, int *data_size,
> +                                    uint8_t second_field)
> +{
> +    BC_STATUS ret;
> +    BC_DTS_PROC_OUT output = {
> +        .PicInfo.width  = avctx->width,
> +        .PicInfo.height = avctx->height,
> +    };
> +    CHDContext *priv = avctx->priv_data;
> +    HANDLE dev       = priv->dev;
> +
> +    *data_size = 0;
> +
> +    // Request decoded data from the driver
> +    ret = DtsProcOutputNoCopy(dev, OUTPUT_PROC_TIMEOUT, &output);
> +    if (ret == BC_STS_FMT_CHANGE) {
> +        av_log(avctx, AV_LOG_VERBOSE, "CrystalHD: Initial format change\n");
> +        avctx->width  = output.PicInfo.width;
> +        avctx->height = output.PicInfo.height;
> +        return RET_COPY_AGAIN;
> +    } else if (ret == BC_STS_SUCCESS) {

It looks strange to me to not have the success case as the first one.

-- 
Ben