[MPlayer-dev-eng] [PATCH] Dvdnav part2 - still frame and simple osd box

[Nico Sabbi]

Ötvös Attila wrote:

>Hi All.
>
>This patch does still frames and simple menu box.
>
>My questions:
>
>What do you think if I modify decode_video() in dec_video.c or
>decode_video_still() remains in vd_videostill.c?
>  
>
there's no need to add vd_videostill.c: if you modify
libavcodec's decoder to release still frames immediately
when it meets a sequence_end code (0x1b7) you will get
the job done cleanly.
Currently you can obtain something similar with:
Index: libmpcodecs/vd_ffmpeg.c
===================================================================
--- libmpcodecs/vd_ffmpeg.c     (revisione 19838)
+++ libmpcodecs/vd_ffmpeg.c     (copia locale)
@@ -204,6 +204,7 @@
     ctx->avctx = avcodec_alloc_context();
     avctx = ctx->avctx;

+avctx->flags|= CODEC_FLAG_LOW_DELAY;
 #ifdef HAVE_XVMC

 #ifdef CODEC_CAP_HWACCEL


but it's not even remotely the right solution


>What do you think  if  the default video codec family can be set to
>ffmpeg in dvdnav stream?
>  
>

not needed: just add -vc ffmpeg12, to your command line/config file

>What do you think if the basic concept is right in: 
>"[PATCH] Dvdnav part1 - color spu menu buttons"?
>http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2006-June/043569.html
>
>Do you have any proposals for the next problem? 
>If the initializiling of the video codec is with still frame (eg. the mplayer 
>comes back from film's playing to dvd menu that is a still frame) then
>still frame was read to init and the mplayer should read again the still
>frame to show but there is only frame and the seek function can't be
>used here.
>
>  
>
>
I don't understand what you mean; please explain

Overall this last patch is many order of magnitude better than the
previous ones you posted, but there are still things I don't like:


1)
+    case STREAM_CTRL_IS_EOF:
+    {
+          *((unsigned int *)arg) = dvdnav_priv->eofstream;
+          return 1;
+    }

not needed: use stream->eof

+    case STREAM_CTRL_IS_STILL_FRAME:
+    {
+          return 1;
+    }

STILLS properties don't belong to the stream layer and code like that is 
nonsense

+    case STREAM_CTRL_DVDNAV_GET_HIGHLIGHT_SHOW:
+    {
+       dvdnav_highlight_event_t highlight_event;
+       dvdnav_get_highlight(dvdnav_priv,&highlight_event,1);
+       (*(mp_highlight_t*)arg).display=highlight_event.display;
+       (*(mp_highlight_t*)arg).sx=highlight_event.sx;
+       (*(mp_highlight_t*)arg).sy=highlight_event.sy;
+       (*(mp_highlight_t*)arg).ex=highlight_event.ex;
+       (*(mp_highlight_t*)arg).ey=highlight_event.ey;
+       (*(mp_highlight_t*)arg).pts=highlight_event.pts;
+       (*(mp_highlight_t*)arg).palette=highlight_event.palette;
+       (*(mp_highlight_t*)arg).buttonN=highlight_event.buttonN;
+       return 1;
+    }
+

again, STILLS properties don't belong to the stream layer, so you can't use
STREAM_CTRL_* for this task.
Use explicit functions instead

Also:
min and max are unnecessarily redefined.
As for the subs drawing code I can't comment: I don't know it.

Thanks,
    Nico