[MPlayer-dev-eng] [PATCH] Fix vd_ffmpeg.c::get_format() to actually query format (take 2)

[Reimar Döffinger]

On Thu, Apr 02, 2009 at 10:46:02AM +0200, Gwenole Beauchesne wrote:
> On Thu, 19 Feb 2009, Reimar Döffinger wrote:
> > On Thu, Feb 19, 2009 at 01:56:54PM +0100, Gwenole Beauchesne wrote:
> >> OK, a workaround, unless someone knows how to fix h264.c, is to make the
> >> query_format() local to vd_ffmpeg.c and implement it as an init_vo() with
> >> avctx->{width,height} set to some defaults if zero (e.g. 64x64),
> >> temporarily. Then, once we know the actual size, VO is reconfigured anyway.
> >
> > This seems fine to me (0x0 never makes sense, so something quite wrong
> > is still better than something totally wrong), but IMO it should be
> > reported as a bug/feature request against FFmpeg.
> 
> A patch is attached. I don't think this is a bug in FFmpeg, it's just that 
> information is not available yet (slice headers are not parsed yet IIRC).

Well, I said bug/feature request. And I think it is important to fix it
if possible, since format support often depends on the resolution,
e.g. Xvideo usually support YV12 only up to 2046x2046, but RGB up to any
resolution (since it does not use hardware overlay for RGB), VDPAU
supports only very specific resolutions for hardware acceleration
(though probably 99% of all videos are covered by those) etc.
Yes, MPlayer does not support such things anyway, but that is no reason
for FFmpeg to do it badly, too.
Which btw. is a bit of a (purely theoretical) issue with your default values,
IIRC VDPAU does not support decoding of 64x64 H.264.
So I am tempted to make a entry on FFmpeg's bug tracker prerequisite for such a
change...

> diff --git a/libmpcodecs/vd_ffmpeg.c b/libmpcodecs/vd_ffmpeg.c
> index f499e8e..16359ec 100644
> --- a/libmpcodecs/vd_ffmpeg.c
> +++ b/libmpcodecs/vd_ffmpeg.c
> @@ -910,6 +910,26 @@ static mp_image_t *decode(sh_video_t *sh, void *data, int len, int flags){
>  }
>  
>  #if CONFIG_XVMC || CONFIG_VDPAU
> +static int query_format(sh_video_t *sh, int fmt)
> +{
> +    vd_ffmpeg_ctx * const ctx = sh->context;
> +    AVCodecContext * const avctx = ctx->avctx;
> +    int r, width, height;
> +
> +    /* Some codecs have not initialized width and height fields yet at
> +     * this point, so we are faking the dimensions so that init_vo()
> +     * doesn't fail because of 0x0 size
> +     */
> +    if ((width = avctx->width) == 0)
> +        avctx->width = 64;
> +    if ((height = avctx->height) == 0)
> +        avctx->height = 64;
> +    r = init_vo(sh, fmt);
> +    avctx->width = width;
> +    avctx->height = height;
> +    return r;
> +}
> +
>  static enum PixelFormat get_format(struct AVCodecContext *avctx,
>                                      const enum PixelFormat *fmt){
>      enum PixelFormat selected_format;
> @@ -921,7 +941,7 @@ static enum PixelFormat get_format(struct AVCodecContext *avctx,
>          imgfmt = pixfmt2imgfmt(fmt[i]);
>          if(!IMGFMT_IS_XVMC(imgfmt) && !IMGFMT_IS_VDPAU(imgfmt)) continue;
>          mp_msg(MSGT_DECVIDEO, MSGL_INFO, MSGTR_MPCODECS_TryingPixfmt, i);
> -        if(init_vo(sh, fmt[i]) >= 0) {
> +        if(query_format(sh, fmt[i]) >= 0) {
>              break;
>          }
>      }

Why don't you just add two lines to init_vo, that is much less ugly.
My idea was something like this:
Index: vd_ffmpeg.c
===================================================================
--- vd_ffmpeg.c (revision 29105)
+++ vd_ffmpeg.c (working copy)
@@ -496,6 +496,10 @@
         width = sh->bih->biWidth>>lavc_param_lowres;
         height = sh->bih->biHeight>>lavc_param_lowres;
     }
+    // HACK: query_format sometimes does not have a resolution set, and
+    // 0-size can not work anyway so this can not break anything
+    if (width  <= 0) width  = 64;
+    if (height <= 0) height = 64;
 
      // it is possible another vo buffers to be used after vo config()
      // lavc reset its buffers on width/heigh change but not on aspect
      // change!!!