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

[Diego Biurrun]

On Sun, Feb 06, 2011 at 04:56:36PM -0700, Philip Langdale wrote:
>
> Here is the updated CrystalHD patch reflecting all feedback given to v3.
>
> Punchline
> ---------
>
> I think it's now possible to seriously consider this for acceptance.
> Yes, there are unresolved problems with 70012 and interlaced handling,
> but the core functionality is sound and genuinely useful. The
> remaining problems can be addressed incrementally.

Nice to hear you are making progress.

Some nitpicks from me below.  Fixing them would be appreciated, but
don't bother sending a fresh patch just for them.

> --- a/configure
> +++ b/configure
> @@ -1235,6 +1236,7 @@ h263_vaapi_hwaccel_select="vaapi h263_decoder"
>  h263i_decoder_select="h263_decoder"
>  h263p_encoder_select="h263_encoder"
>  h264_decoder_select="golomb h264dsp h264pred"
> +h264_crystalhd_decoder_select="crystalhd h264_mp4toannexb_bsf"
>  h264_dxva2_hwaccel_deps="dxva2api_h"
>  h264_dxva2_hwaccel_select="dxva2 h264_decoder"
>  h264_vaapi_hwaccel_select="vaapi"
> @@ -1256,10 +1258,13 @@ mpeg2video_encoder_select="aandct"
>  mpeg4_decoder_select="h263_decoder mpeg4video_parser"
>  mpeg4_encoder_select="h263_encoder"
>  mpeg_vdpau_decoder_select="vdpau mpegvideo_decoder"
> +mpeg1_crystalhd_decoder_select="crystalhd"
>  mpeg1_vdpau_decoder_select="vdpau mpeg1video_decoder"
> +mpeg2_crystalhd_decoder_select="crystalhd"
>  mpeg2_dxva2_hwaccel_deps="dxva2api_h"
>  mpeg2_dxva2_hwaccel_select="dxva2 mpeg2video_decoder"
>  mpeg2_vaapi_hwaccel_select="vaapi mpeg2video_decoder"
> +mpeg4_crystalhd_decoder_select="crystalhd"
>  mpeg4_vaapi_hwaccel_select="vaapi mpeg4_decoder"
>  mpeg4_vdpau_decoder_select="vdpau mpeg4_decoder"
>  mpeg_xvmc_decoder_deps="X11_extensions_XvMClib_h"
> @@ -1270,6 +1275,7 @@ msmpeg4v2_decoder_select="h263_decoder"
>  msmpeg4v2_encoder_select="h263_encoder"
>  msmpeg4v3_decoder_select="h263_decoder"
>  msmpeg4v3_encoder_select="h263_encoder"
> +msmpeg4_crystalhd_decoder_select="crystalhd"
>  nellymoser_decoder_select="mdct"
>  nellymoser_encoder_select="mdct"
>  png_decoder_select="zlib"
> @@ -2798,6 +2808,7 @@ done
>  
>  check_lib math.h sin -lm
>  disabled vaapi || check_lib va/va.h vaInitialize -lva
> +disabled crystalhd || check_lib libcrystalhd/libcrystalhd_if.h DtsGetVersion -lcrystalhd

alphabetical order

nit: _ goes before all letters in the FFmpeg alphabet ;)

> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -141,13 +142,17 @@ void avcodec_register_all(void)
>      REGISTER_ENCDEC  (MPEG1VIDEO, mpeg1video);
>      REGISTER_ENCDEC  (MPEG2VIDEO, mpeg2video);
>      REGISTER_ENCDEC  (MPEG4, mpeg4);
> +    REGISTER_DECODER (MPEG4_CRYSTALHD, mpeg4_crystalhd);
>      REGISTER_DECODER (MPEG4_VDPAU, mpeg4_vdpau);
>      REGISTER_DECODER (MPEGVIDEO, mpegvideo);
>      REGISTER_DECODER (MPEG_VDPAU, mpeg_vdpau);
> +    REGISTER_DECODER (MPEG1_CRYSTALHD, mpeg1_crystalhd);
>      REGISTER_DECODER (MPEG1_VDPAU, mpeg1_vdpau);
> +    REGISTER_DECODER (MPEG2_CRYSTALHD, mpeg2_crystalhd);
>      REGISTER_ENCDEC  (MSMPEG4V1, msmpeg4v1);
>      REGISTER_ENCDEC  (MSMPEG4V2, msmpeg4v2);
>      REGISTER_ENCDEC  (MSMPEG4V3, msmpeg4v3);
> +    REGISTER_DECODER (MSMPEG4_CRYSTALHD, msmpeg4_crystalhd);
>      REGISTER_DECODER (MSRLE, msrle);

ditto

> --- /dev/null
> +++ b/libavcodec/crystalhd.c
> @@ -0,0 +1,852 @@
> +
> +typedef enum {
> +    RET_ERROR = -1,
> +    RET_OK = 0,
> +    RET_COPY_AGAIN = 1,
> +    RET_SKIP_NEXT_COPY = 2,
> +} CopyRet;

nit: Align the '='.

> +/* Need typedef to allow next pointer. */
> +typedef struct OpaqueList OpaqueList;

The comment makes no sense to me, I'd just drop the typedef.
Also, CamelCase is ugly.

> +static inline void memcpy_pic(void *dst, const void *src,
> +                              int bytesPerLine, int height,
> +                              int dstStride, int srcStride)
> +{
> +    int i;
> +    for (i = 0; i < height; i++) {
> +        memcpy(dst, src, bytesPerLine);
> +        src = (const uint8_t*)src + srcStride;
> +        dst = (uint8_t*)dst + dstStride;
> +    }
> +}

Question for the audience: Don't we have something like that already?

> +    if (priv->head == NULL) {

nit: (!priv->head)

more below

> +            bsfc= av_bitstream_filter_init("h264_mp4toannexb");

nit: space around '='

> +    interlaced        = ((output->PicInfo.flags & VDEC_FLAG_INTERLACED_SRC) &&
> +                         !(output->PicInfo.flags & VDEC_FLAG_UNKNOWN_SRC)) ||
> +                        next_frame_same || bottom_field || second_field;

Indentation is off by one.

> +    if (output->PicInfo.timeStamp != 0) {
> +        priv->pic.pkt_pts =
> +            opaque_list_pop(priv, output->PicInfo.timeStamp);

nit: unnecessary linebreak

> +        av_log(avctx, AV_LOG_INFO,
> +               "CrystalHD: No frames ready. Returning\n");

ditto

Diego