[FFmpeg-devel] [RFC] libavfilter-soc: direct rendering

[Stefano Sabatini]

On date Thursday 2009-06-11 01:51:00 +0200, Michael Niedermayer encoded:
> On Thu, Jun 11, 2009 at 01:20:35AM +0200, Stefano Sabatini wrote:
[...]
> > Attached patches implement this new system, try them with:
> > ffplay in.avi -vfilters "pad = exp_w=in_w+30 : exp_h=in_h + 30 : color = red,
> >                          pad = exp_w=in_w+30 : exp_h=in_h + 30 : color = blue,
> >                          pad = exp_w=in_w+30 : exp_h=in_h + 30 : color = yellow"
> > 
> > (no I didn't tested them extensively).
> 
> more testing that mixes pad, crop and scale is definitly recommanded ...
> also testing with non trivial chains that have loops is a good idea
> but i guess you know that too ...
> 
> that brings up the point of the very very important regression tests
> we need some, it would make testing things like this easier

Yes, I agree this is something we need at this point.  I'll try to
think at the test to implement, meaningwhile ideas are welcome.

> > Attached patches, *does not still implement direct rendering*, direct
> > rendering should be made possible using this new get_video_buffer() API.
> 
> it does implement direct rendering between filters just not with the decoder
> and vo
> one issue might be that the decoder requests frames in a order
> different from what it outputs
> 
> 
> > 
> > Let me consider the specific example of ffplay.c, which currently
> > does:
> > 
> >     while (!(ret = get_video_frame(priv->is, priv->frame, &pts, &pkt)))
> >         av_free_packet(&pkt);
> >     if (ret < 0)
> >         return -1;
> > 
> >     /* FIXME: until I figure out how to hook everything up to the codec
> >      * right, we're just copying the entire frame. */
> >     picref = avfilter_get_video_buffer(link, AV_PERM_WRITE, 0, 0, link->w, link->h);
> >     av_picture_copy((AVPicture *)&picref->data, (AVPicture *)priv->frame,
> >                     picref->pic->format, picref->w, picref->h);
> >     av_free_packet(&pkt);
> > 
> > We want to avoid the initial memcpy, furthermore if the last frame is
> > an output device it may directly return a buffer from the display
> > buffer area, thus allowing for direct rendering.
> > 
> > Ideally we should be able to pass to get_video_frame() /
> > avcodec_decode_video2() the buffer when we want the decoded buffer to
> > be written.
> > 
> > Hints regarding the correct approach to follow are much appreciated, I
> 
> > suppose I need to use the AVCodecContext::{get|release}_buffer, right?
> 
> yes i think so
> 
> 
> [...]
> > Index: libavfilter-soc/ffmpeg/libavfilter/avfilter.h
> > ===================================================================
> > --- libavfilter-soc.orig/ffmpeg/libavfilter/avfilter.h	2009-06-11 00:14:27.000000000 +0200
> > +++ libavfilter-soc/ffmpeg/libavfilter/avfilter.h	2009-06-11 00:28:49.000000000 +0200
> > @@ -88,6 +88,8 @@
> >      int linesize[4];            ///< number of bytes per line
> >      int w;                      ///< image width
> >      int h;                      ///< image height
> > +    int padleft, padtop;
> > +    int exp_w, exp_h;
> >  
> >      int64_t pts;                ///< presentation timestamp in units of 1/AV_TIME_BASE
> >  
> 
> can you generate patches so they show the function / struct name that is
> changed?
> that -p flag ...

uhm using quilt which is purely diff/patch based there is no way I
think, well maybe it's time to me to swith to git...
  
> > @@ -291,7 +293,7 @@
> >       *
> >       * Input video pads only.
> >       */
> > -    AVFilterPicRef *(*get_video_buffer)(AVFilterLink *link, int perms);
> > +    AVFilterPicRef *(*get_video_buffer)(AVFilterLink *link, int perms, int padleft, int padtop, int exp_w, int exp_h);
> >  
> >      /**
> >       * Callback called after the slices of a frame are completely sent. If
> > @@ -359,7 +361,9 @@
> >  int avfilter_default_config_input_link (AVFilterLink *link);
> >  /** default handler for get_video_buffer() for video inputs */
> >  AVFilterPicRef *avfilter_default_get_video_buffer(AVFilterLink *link,
> > -                                                  int perms);
> > +                                                  int perms,
> > +                                                  int padleft, int padtop,
> > +                                                  int exp_w, int exp_h);
> >  /**
> >   * A helper for query_formats() which sets all links to the same list of
> >   * formats. If there are no links hooked to this filter, the list of formats is
> > @@ -500,7 +504,7 @@
> >   * @return      A reference to the picture. This must be unreferenced with
> >   *              avfilter_unref_pic when you are finished with it.
> >   */
> > -AVFilterPicRef *avfilter_get_video_buffer(AVFilterLink *link, int perms);
> > +AVFilterPicRef *avfilter_get_video_buffer(AVFilterLink *link, int perms, int padleft, int padtop, int exp_w, int exp_h);
> 
> the new arguments need to be documented

In attachment a try, writing the docs for them I realized that maybe
it would be better to call the params shiftright / shiftbottom.

> [...]
> 
> > +const char *var_names[]= {
> 
> static 

Fixed.
 
> > +    "PI",
> > +    "E",
> > +    "in_w",     ///< input width
> > +    "exp_w",    ///< expanded width
> 
> is out_w a poor choice for some reason ?

Thinking at it out_[wh] are better names, so changed vf_pad
accordingly.

Attached patches are not ready for review, consider it a
backup/proof-of-concept, still many points need to be addressed, in
particular:
 
* vflip filter is broken with the patch applied, the pad filter needs
  to be modified to work with negative logic too if possible, in the
  worst possible case maybe the vflip shouldn't be made slice-friendly
  as currently is.

* More docs to the new added fields are needed

* I suspect the new get_video_buffer API won't work with non-planar
  formats, also maybe a pixel format parameter has to be added to the
  params list.

* this patchset still doesn't implement direct rendering, both
  vsrc_movie/vsrc_buffer/ffplay need to be changed accordingly, but
  this may be a separate step.
 
* a lavfi regression test is required.

Feel free to have a look at any of these.

Regards.
-- 
FFmpeg = Friendly & Fierce Maxi Powered Extroverse Gem
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lavfi-recursive-get-video-buffer.patch
Type: text/x-diff
Size: 14540 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090618/70dacbb0/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pad-implement.patch
Type: text/x-diff
Size: 14075 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090618/70dacbb0/attachment-0001.patch>