[MPlayer-dev-eng] [PATCH] RFC: CrystalHD decoder support

[Reimar Döffinger]

On Sun, Dec 19, 2010 at 06:50:04PM -0700, Philip Langdale wrote:
> However, the biggest issue is that the decoder breaks an assumption
> in mplayer about how many
> calls to decode can occur before a frame is returned. At the start
> up, the crystalhd might
> require 20 or more calls before it ever returns a frame, and without
> increasing the maximum
> number of buffered_pts, it will start discarding them, ensuring that
> av-sync goes out the window.

Really? That amount of buffered frames would mean this hardware is completely
useless for video telephone or any other interactive application.

> So, I've bumped max buffered_pts to 128 in this patch - 64 was too
> low, but it might need to be
> even higher for 1080p content.

This is more than 4 seconds, you are going to run into _serious_ issues,
with that, e.g. if combined with uncompressed multichannel PCM audio
4 seconds is probably more than MPlayer will be able to buffer, causing
playback to fail.

> I'm also not sure how
> QUERY_UNSEEN_FRAMES should be used. I don't
> know how many complete frames are in the decoder waiting to be
> decoded

Why don't you know that? At least for the non-interlaced, non-error-concealment
case it should be just a matter of "number of packets in - number of frames out".

> and mplayer seems to be
> comparing the result with buffered_pts which comes from the pts
> passed to each decode call,
> so I'm pretty confused.

Figure out how timestamp reordering is supposed to be done, then the right
solution should become more obvious.
Unless of course they went the cheap way and don't have any way to figure out
with input time stamp belongs to which output frame, in which case that solution
would become completely useless for any variable frame-rate content...

> It also doesn't help with vd_ffmpeg adding
> 10 and then having it subtracted
> later. So I ended up returning 20 + the number of decode calls that
> haven't returned a frame since
> the last flush. It seems to work but it's hardly scientific.

A too large value will work most of the time, but is likely to cause
issues with timestamp wraps.

> With that in place, progressive <= 720p content plays well - with a
> note that I have to force lavf
> to demux avi and wmv files correctly. The native demuxers dont' sync
> properly - i think because
> they also make assumptions about how long you have to wait for
> frames to come back.

demuxers aren't concerned with what comes after. Take a look
at mpi_no_picture in vd_ffmpeg.c, it's likely you have to do
something like that (though actually that should only
really apply to the interlaced case you mention below).
Hm, I just saw you actually use that, but I have some doubts
if it is correct to use it for the ReadyListCount case.
And the behaviour of the native demuxers and lavf should behave
the same if you use -nocorrect-pts.

> There's
> also no real benefit in terms of code sharing as the only place we
> need codec awareness is the
> sps/pps decoding and ffmpeg's h.264 codec doesn't build up a
> compatible struct anyway.

The point would be to have code sharing with e.g. VLC, at least in
the long run.
There is not enough manpower available in the MPlayer project
to maintain this, I guarantee you that the moment you don't actively
look after it it will bit-rot and become unusable in no time,
while in the process probably wasting a good bit of mine and other's
time.

> Philosophically, I think it can be argued either way; you can
> consider libcrystalhd to be a library
> codec like theora or xvid and say it's a valid mplayer codec on that
> basis,

No, it is not anymore. FFmpeg is the only valid place to add codec
support, no matter what.

> --- a/Makefile
> +++ b/Makefile
> @@ -330,6 +330,8 @@ SRCS_COMMON-$(XMMS_PLUGINS)          += libmpdemux/demux_xmms.c
>  SRCS_COMMON-$(XVID4)                 += libmpcodecs/vd_xvid4.c
>  SRCS_COMMON-$(ZR)                    += libmpcodecs/vd_zrmjpeg.c \
>                                          libmpcodecs/vf_zrmjpeg.c
> +SRCS_COMMON-$(CRYSTALHD)             += libmpcodecs/vd_crystalhd.c
> +

That is not quite where the letter "c" goes in my alphabet...

> @@ -7023,6 +7027,21 @@ else
>  fi
>  echores "$_libdv"
>  
> +echocheck "CrystalHD"
> +if test "$_crystalhd" = auto ; then
> +   _crystalhd=no
> +   statement_check_broken libcrystalhd/bc_dts_types.h libcrystalhd/libcrystalhd_if.h 'DtsGetVersion(0, 0, 0)' -lcrystalhd $_ld_lm && _crystalhd=yes

Can they be convinced to fix their header instead?

>      /* Please do not add any new decoders here. If you want to implement a new
>       * decoder, add it to libavcodec, except for wrappers around external
>       * libraries and decoders requiring binary support. */

Ah, ok, that is where your interpretation came from.
I guess it is time to remove the wrapper exception, or at least reduce it
to "trivial wrappers".

> + * MPlayer is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

With regards to FFmpeg integration a LGPL version would be preferable.

> + * fast_memcpy: From libcrystalhd

MPlayer already has a fast_memcpy function (two actually, depending on
whether the source/target is normal memory or a memory-mapped device).
And in difference to this variant using intrinsics it
1) will have predictable performance (yes, compilers still sometimes compile
   intrinsics in a way that negates any speed advantage)
2) it actually compiles on older compilers

> +  uint8_t *data = extradata;
> +  uint32_t data_size = extradata_size;

I fail to see what these are needed for.

> +  if(*(char *)extradata == 1) {

Same for that cast/

> +     priv->nal_length_size = ((*(((char*)(data))+4))&0x03)+1;

And this should be written in a way that doesn't look worse than LISP.

> +  profile = (data[1] << 16) | (data[2] << 8) | data[3];

AV_RB24

> +    nal_size = (data[0] << 8) | data[1];

AV_RB16

> +      return priv->is_avc ? BC_MSUBTYPE_AVC1 : BC_MSUBTYPE_H264;

AVC1? Worst naming ever. Nothing you can do about that though, except
adding a comment what they actually mean by that.
And of course not name your variable is_avc, this is bytestream (aka
Annex B) vs. NAL (aka mp4) format.

> +                if (((((uintptr_t) dst) & 0xf) == 0) && ((((uintptr_t) src) & 0xf) == 0))
> +                {
> +                        while (count >= (16*4))

Wow, why not add a few () more, I'm sure it's possible to make it completely unreadable...

> +         int format =(*((int *)arg));

= *(int *)arg;
works just fine, it's not like there's even the remotest possibility
to evaluate that in any other order...

> +	Priv* priv;

Tab.

> +	switch(sh->codec->outfmt[sh->outfmtidx]){
> +	case IMGFMT_YUY2:
> +		break;
> +	default:
> +		mp_msg(MSGT_DECVIDEO, MSGL_V, "Unsupported out_fmt: 0x%X\n",
> +		       sh->codec->outfmt[sh->outfmtidx]);
> +		return 0;
> +	}

Pointless, query_format already checks that the format is supported.
Also, why only IMGFMT_YUY2? That wastes a huge amount of bandwidth
(unless the input is actual 4:2:2 - if the is even supported).

> +	/* Gather some information about the host library */
> +	/* Initialize the library */

More tabs.

> +        priv->is_avc = extradata_size > 0 ? (*(char *)extradata == 1) : 0;

extradata_size > 0 && *extradata == 1;

> +        format.height = sh->disp_h;
> +        if (format.height == 1088) {
> +           format.height = 1080;
> +        }

That certainly isn't correct.

> +   ret = DtsSetInputFormat(priv->dev, &format);
> +   if (ret != BC_STS_SUCCESS) {
> +      mp_msg(MSGT_DECVIDEO, MSGL_V, "CrystalHD: SetInputFormat failed\n");
> +      return 0;
> +   }

Shouldn't you do some stuff like closing the device and freeing some stuff
in all those error cases?

> +    ret = DtsSetColorSpace(priv->dev, OUTPUT_MODE422_YUY2);

Probably worst possible choice of output color format, it is not compact
(16 bpp vs. 12 bpp), it is not the final format (that will usually be RGB),
it is not supported by all vos (e.g. with -vo gl that will always imply a
software colorspace conversion), and it will involve chroma interpolation
on the device - chances are good it's bad if not outright broken.

> +        free(priv->sps_pps_buf);
> +        free(priv);
> +static mp_image_t* decode(sh_video_t *sh, void* data, int len, int flags)
> +   BC_STATUS ret;
> +   BC_DTS_PROC_OUT output;

Inconsistent indentation, inconsistent placement of *

> +   if (flags != 0) {
> +      if (len != 0) {

The != 0 is fairly redundant

> +      /*
> +       * No frames ready. Don't try to extract.
> +       */
> +      if (decoder_status.ReadyListCount == 0) {
> +         usleep(1000);

Sleeping in the decoder is going to be rather suboptimal.

> +      sh->disp_h = output.PicInfo.height;
> +      if (output.PicInfo.height == 1088) {
> +         sh->disp_h = 1080;
> +      }

This is still just broken. You will probably want the cropped dimensions.
However if the hardware does not support cropping on its own there's
going to be a lot of issues.

> +          interlaced =  (output.PicInfo.flags & VDEC_FLAG_INTERLACED_SRC);// &&
> +                       //!(output.PicInfo.flags & VDEC_FLAG_UNKNOWN_SRC);
> +          top_field = (output.PicInfo.flags & VDEC_FLAG_TOPFIELD) == VDEC_FLAG_TOPFIELD;
> +          bottom_field = (output.PicInfo.flags & VDEC_FLAG_BOTTOMFIELD) == VDEC_FLAG_BOTTOMFIELD;

I have quite some doubts about this.
The obvious one is: Why is the API designed in a way that you can end up with the
two top_field == bottom_field cases and what are they supposed to mean.
The less obvious one is: what exactly does interlaced mean?
There is PAFF coding, there is MBAFF coding and then interlaced can be
coded as progressive (even if it is a bad idea), what exactly does this
one indicate (I have a suspicion that it actually mean PAFF).

> +            for (sY = 0; sY < height; dY++, sY++) {
> +               fast_memcpy(&(dst[dY * dStride]), &(src[sY * width]), width);
> +               if (interlaced) {
> +                  dY++;
> +               }
> +            }

I suspect that memcpy_pic is going to solve this easier.
Still, a memcpy is not exactly fast at high resolutions.