[FFmpeg-devel] Patch: CrystalHD decoder support

Philip Langdale philipl
Tue Dec 28 18:46:17 CET 2010


On Tue, 28 Dec 2010 10:03:25 +0000 (UTC)
Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:

> Philip Langdale <philipl <at> overt.org> writes:
> 
> ...
> 
> > I'm attaching both the main ffmpeg patch and the mplayer patch
> > required to use it from mplayer.
> 
> To reduce the confusion, I would suggest to remove the MPlayer part.
> Transcoding with ffmpeg works, no?

Yes, although it's a highly questionable activity; The hardware can only
decode to YUY2, so you waste your time converting back to YUV420 and I
doubt it's a lossless conversion.
 
> ...
> 
> > -HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h
> > +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h
> > vdpau.h xvmc.h
> 
> How is this related?

It isn't. That's leftover from my previous iteration as an hwaccel using
Gwenole's v2 patch. It turns out this is not a good fit for the
interface so I redid it as a full decoder.
 
> ...
> 
> > + * - CrystalHD decoder module -
> > + *
> > + * Copyright(C) 2010 Philip Langdale <ffmpeg.philipl <at>
> > overt.org>
> > + *
> > + * Credits:
> > + * extract_sps_pps_from_avcc: from xbmc
> > + *
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> 
> This is the only serious part of my comments here:
> Did you ask somebody from xbmc if you may re-license
> extract_sps_pps_from_avcc? We try very hard to hunt copyright
> violators, it would be unfortunate if it also happened to us...

Sorry - I was sloppy here; the function originally comes from
gstbcmdec (the gstreamer filter) which is LGPL. I will change the
credits accordingly.

> ...
> 
> > +#define _GNU_SOURCE
> 
> I suspect you should explain why this is needed.

It's needed to get usleep(). I will add a comment.

> ...
> 
> > +#ifdef CONFIG_FASTMEMCPY
> > +#include "libvo/fastmemcpy.h"
> > +#else
> > +#define fast_memcpy(d, s, l) memcpy(d, s, l)
> > +#endif
> 
> I would suggest to remove this hunk.

Do you think there's insufficient benefit in fast_memcpy to bother
taking advantage of it? I'll remove the usage if you don't want it
but it seems desirable to use it if it's there.

> 
> > +static inline int receive_frame(AVCodecContext *avctx, void *data,
> > +                                int *data_size);
> > +static inline int copy_frame(AVCodecContext *avctx,
> > BC_DTS_PROC_OUT *output,
> > +                             void *data, int *data_size);
> 
> Please re-order your functions to make this unneeded.

Will do.

> ...
> 
> > +    for (unsigned int i = 0; i < num_pps; i++)
> > +    {
> 
> On one line.

Will do. This is the original code style of the function.

> > +        if (data_size < 2) {
> > +            return -1;
> > +        }
> 
> Most people here think that the braces are unneeded and reduce
> readability.

Will do.

Thanks,

--phil



More information about the ffmpeg-devel mailing list