[FFmpeg-devel] [PATCH]NVIDIA VDPAU patch for h264
Stephen Warren
swarren
Mon Dec 1 20:17:06 CET 2008
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.
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.
> 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?
> 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.
> [...]
> > +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.
The code is duplicated everywhere because this check is relevant everywhere that a picture's data pointer is "converted" to a VDPAU render structure, to ensure that client code didn't pass in an invalid picture structure at some point.
Reimar D?ffinger wrote:
> On Sun, Nov 30, 2008 at 10:03:39PM +0100, Carl Eugen Hoyos wrote:
> > +extern int ff_VDPAU_h264_add_data_chunk(H264Context *h, const uint8_t
> *buf, int buf_size);
> > +extern int ff_VDPAU_h264_picture_complete(H264Context *h);
> > +
>
> extern is useless should not be used here IMO.
We modeled this on mpeg12.c's code for declaring the XvMC functions.
Anyway, since the function are implemented in a different file, it seems they should be declared extern..
> > +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.
> > @@ -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.
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.
> > @@ -7384,8 +7431,26 @@
> > && (avctx->skip_frame < AVDISCARD_NONREF || hx-
> >nal_ref_idc)
> > && (avctx->skip_frame < AVDISCARD_BIDIR || hx-
> >slice_type_nos!=FF_B_TYPE)
> > && (avctx->skip_frame < AVDISCARD_NONKEY || hx-
> >slice_type_nos==FF_I_TYPE)
> > - && avctx->skip_frame < AVDISCARD_ALL)
> > + && avctx->skip_frame < AVDISCARD_ALL) {
> > +#ifdef CONFIG_VDPAU
> > + if (avctx->vdpau_acceleration) {
>
> And adding a ENABLE_VDPAU in MPlayer is a complete non-issue.
> Given that very few people (at least developers) will compile FFmpeg
> with this enabled, it is _very_ advisable to make the compiler at least
> syntax-check as much of it as possible.
We are working on converting our code to use ENABLE_VDPAU. That change simply didn't make it in time for this release of our patches. A future version of our patches will do this.
> > @@ -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?
> > + render->info.h264.field_pic_flag = (s-
> >picture_structure != PICT_FRAME) ? 1 : 0;
> > + render->info.h264.bottom_field_flag = (s-
> >picture_structure == PICT_BOTTOM_FIELD) ? 1 : 0;
>
> Uh, like useless code much? You could add ten more "? 1 : 0" and the
> result still would not change a bit...
Technically, the compiler is free to generate any non-zero value for comparison results, hence the "conversion" code, to ensure consistent values. Consistency helps when printing out and comparing dumps of structures to look for problems.
> On Mon, Dec 01, 2008 at 09:42:25AM -0500, compn wrote:
> > can the vdpau api handle decoding most/all h264 resolutions/bitrate etc?
>
> All: that is impossible.
> All relevant ones: probably depends on the hardware.
>
> > is ffmpeg asking for a generic decoder instead of a per-profile decoder?
>
> IMO definitely. I do not know for sure what the point of splitting by
> profiles was for VDPAU, but I very much doubt it is necessary to expose
> this anywhere that does not directly use the VDPAU.
> An application might want to be able to get some additional information
> about the stream to decide if it should try to use a hardware decoder or
> not, but this then should be done in a more generic way.
> E.g. the current approach would completely fail is somebody concatenated
> a "high" with a "baseline" profile H.264. If this information was
Yes, this would fail, and this is what we would expect.
The H.264 spec allows a conformant HIGH profile decoder not to implement certain features required by BASELINE. Simply throwing a HIGH and a BASELINE stream into the same file will therefore yield a non-conformant stream; something that is neither HIGH nor BASELINE profile.
nvpublic
More information about the ffmpeg-devel
mailing list