[FFmpeg-devel] [PATCH]NVIDIA VDPAU patch for h264
Michael Niedermayer
michaelni
Mon Dec 1 21:57:11 CET 2008
On Mon, Dec 01, 2008 at 11:17:06AM -0800, Stephen Warren wrote:
> Hi. I work for NVIDIA on VDPAU.
>
> First off, our current position on the VDPAU patches is that we're still working on them and are aware they aren't ready for inclusion in ffmpeg/MPlayer as-is. However, we have been benefiting from the discussion of the patches here, and have been implementing some of the feedback.
>
> I'd like to respond to a few of the comments:
>
> Michael Niedermayer wrote:
> > > @@ -101,6 +104,16 @@
> > > {0,2,0,2,7,10,7,10}
> > > };
> > >
> > > +static const enum PixelFormat pixfmt_vdpau_h264_baseline_420[] = {
> > > + PIX_FMT_VDPAU_H264_BASELINE,
> > > + PIX_FMT_NONE};
> > > +static const enum PixelFormat pixfmt_vdpau_h264_main_420[] = {
> > > + PIX_FMT_VDPAU_H264_MAIN,
> > > + PIX_FMT_NONE};
> > > +static const enum PixelFormat pixfmt_vdpau_h264_high_420[] = {
> > > + PIX_FMT_VDPAU_H264_HIGH,
> > > + PIX_FMT_NONE};
> > > +
> >
> > i still do not understand why these are split per profile
>
> There are a couple of reasons:
>
> 1) We were inspired by the model of the XvMC integration into ffmpeg. This separates IDCT v.s. MoCo into separate PIX_FMT values. Admittedly the data format for those is more different than for different H.264 profiles.
MPEG2 also has profiles, XvMC doesnt have per profile pixfmts ...
>
> 2) The data formats really are different; there are syntax elements or features allowed in H.264 BASELINE profile that are not allowed in MAIN or HIGH profiles (for example, FMO, ASO, RS). A conformant decoder for MAIN is allowed to solely implement the features required for MAIN, and omit those required for BASELINE.
>
> 3) Due to (2) above, the VDPAU API exposes the ability to specifically create a decoder object that is capable of decoding a specific profile. NVIDIA's implementation of VDPAU only supports MAIN and HIGH for example.
So a BASELINE file would fail, because you explicitly let it fail.
The file could use some feature that is allowed in BASELINE but not MAIN
and that your decoder doesnt support but more likely the file doesnt.
I didnt do a survey of h264 files, but the only ones ive seen with
FMO for example are test bitstreams from JVT.
>
> > There is no fallback to the SW decoder if one profile isnt supported,
>
> Is such a mechanism already implemented in ffmpeg? I don't believe the XvMC path automatically falls back to software?
no
>
> > The profile should be available through AVCodecContext for a user app to
> > decide what to do ...
>
> We can investigate pushing the VDPAU profile selection into MPlayer's vo_vdpau module if you still believe this makes sense, after seeing the above explanation.
as reimar did not seem to be too happy about failing based on profiles
i suspect such checks would not be too welcome on mplayer-dev either
>
> > [...]
> > > +static int VDPAU_h264_set_reference_frames(H264Context *h)
> > > +{
> > > + MpegEncContext * s = &h->s;
> > > + vdpau_render_state_t * render, * render_ref;
> > > + VdpReferenceFrameH264 * rf, * rf2;
> > > + Picture * pic;
> > > + int i, list;
> > > +
> > > + render = (vdpau_render_state_t*)s->current_picture_ptr->data[2];
> >
> > > + assert(render != NULL);
> > > + assert(render->magic == MP_VDPAU_RENDER_MAGIC);
> > > + if ((render == NULL) || (render->magic != MP_VDPAU_RENDER_MAGIC))
> > > + return -1; // make sure that this is render packet
> >
> > assert(!A)
> > if(A)
> >
> > also this code is duplicated all over the place ...
>
> Again, we followed the XvMC model. We'll certainly fix up the syntax change you specify.
well either A can be NULL or it cant
if it can be NULL the assert() is wrong, it would kill the application
if it cant the if(==NULL) makes no sense.
[...]
> > > +static const enum PixelFormat pixfmt_vdpau_h264_baseline_420[] = {
> > > + PIX_FMT_VDPAU_H264_BASELINE,
> > > + PIX_FMT_NONE};
> > > +static const enum PixelFormat pixfmt_vdpau_h264_main_420[] = {
> > > + PIX_FMT_VDPAU_H264_MAIN,
> > > + PIX_FMT_NONE};
> > > +static const enum PixelFormat pixfmt_vdpau_h264_high_420[] = {
> > > + PIX_FMT_VDPAU_H264_HIGH,
> > > + PIX_FMT_NONE};
> >
> > I agree with Michael that this seems very ugly and "painful". I could
> > imagine that it makes fallback to software easier for MPlayer, but that
> > special case is not enough to justify it.
> > Also from what I heard generally, the profile information in the file
> > is generally useless anyway, at least it can not be relied on, making
> > this even more questionable.
>
> Certainly, some metadata in file headers is often incorrect (e.g. level, max number of reference frames). I certainly hope that the profile value specifically isn't, because as I mentioned above, the syntax of different profiles can be different.
its not only about profile being wrong but also that a file that is both
BASELINE & MAIN compatible is possibly marked as BASELINE
>
> > > @@ -2142,10 +2155,8 @@
> > > s->quarter_sample = 1;
> > > s->low_delay= 1;
> > >
> > > - if(avctx->codec_id == CODEC_ID_SVQ3)
> > > - avctx->pix_fmt= PIX_FMT_YUVJ420P;
> > > - else
> > > - avctx->pix_fmt= PIX_FMT_YUV420P;
> > > + // Set in decode_postinit() once initial parsing is complete
> > > + avctx->pix_fmt = PIX_FMT_NONE;
> >
> > What is the point of moving the SVQ3 case, I doubt the hardware
> > acceleration supports that?
> > Also without the profile stuff the pix_fmt could still be set here...
>
> I'm not actually familiar with what SVQ3 is. Any explanation would be helpful.
see libavcodec/svq3.c
>
> We applied the same set of changes to all the codecs in h264.c, in an attempt to keep everything consistent. Is this not the correct thing to do?
>
> > > @@ -7257,6 +7301,9 @@
> > > H264Context *hx;
> > > int i;
> > >
> > > + if(avctx->vdpau_acceleration) {
> > > + return;
> > > + } else
> >
> > Useless {} and else
>
> Well, the {} is a matter of coding style. Is there a document for ffmpeg that we can follow?
>
> Anyway, we tend to add {} on all blocks, in order to prevent issues if somebody later adds extra statements to the block and forgets to add {}. I think our reading of existing ffmpeg code showed that this was normal in ffmpeg too.
{} or lack thereoff is very very minor issue and IMHO i would only
omit {} in the absense of "else" in the above case. But again this really
doesnt matter in relation to patch acceptance
[...]
> > > @@ -7973,4 +8047,35 @@
> > > .long_name = NULL_IF_CONFIG_SMALL("H.264 / AVC / MPEG-4 AVC / MPEG-4
> > part 10"),
> > > };
> > >
> > > +#ifdef CONFIG_VDPAU
> > > +static av_cold int h264_vdpau_decode_init(AVCodecContext *avctx){
> > > + if( avctx->thread_count > 1)
> > > + return -1;
> > > + if( !(avctx->slice_flags & SLICE_FLAG_CODED_ORDER) )
> > > + return -1;
> > > + if( !(avctx->slice_flags & SLICE_FLAG_ALLOW_FIELD) ){
> > > + dprintf(avctx, "h264.c: VDPAU decoder does not set
> > SLICE_FLAG_ALLOW_FIELD\n");
> > > + }
> >
> > IMO these each need a comment why and possibly why return -1 is the best
> > solution (e.g. would setting thread_count to 1 instead be
> > acceptable/better?).
>
> OK. We can certainly just override the values instead. For the two flags, can we just remove them from avctx->slice_flags, or would that break something?
changing the value of the slice flags will not work unless they are set
incorrectly to begin with or their value didnt matter for the file and
how the decoder is used ...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081201/ff2623bb/attachment.pgp>
More information about the ffmpeg-devel
mailing list