[MPlayer-dev-eng] [PATCH] print interlacing info on decode

[Diego Biurrun]

On Wed, Sep 15, 2010 at 09:59:00PM +0200, Jarek Czekalski wrote:
>  Ok, I'm posting again in a more proper manner, even including an  
> attachement. This is the same I posted yesterday, except that I cleared  
> the warning: no prototype.

Your workaround for the warning is not acceptable.

> I don't know where to put this prototype. This function shouldn't be  
> visible outside the file, so I put the prototype just before the 
> definition.

Functions not used outside of one compilation unit should be marked
as 'static'.

> Could you review it and maybe commit?

I haven't thought about the idea yet, but here are some comments
for the implementation:

> --- libmpcodecs/dec_video.c	(revision 32249)
> +++ libmpcodecs/dec_video.c	(working copy)
> @@ -52,10 +52,41 @@
>  
> +    // report scan type
> +    int is_unknown = !(mpi->fields & MP_IMGFIELD_ORDERED);
> +    int is_interlaced = ((mpi->fields & MP_IMGFIELD_INTERLACED) != 0);

This could be more readable with the = aligned.

> +    if (is_unknown) {
> +        mp_msg(MSGT_DECVIDEO, MSGL_V, "%s unknown\n", header);
> +        }
> +    else {
> +        if (is_interlaced) {
> +            mp_msg(MSGT_DECVIDEO, MSGL_V, "%s interlaced, %s\n", header,
> +                   (mpi->fields & MP_IMGFIELD_TOP_FIRST) ? "tff" : "bff");
> +            }
> +        else { // progressive
> +            mp_msg(MSGT_DECVIDEO, MSGL_V, "%s progressive\n", header);
> +            }
> +        }

Your } placement is weird.

> @@ -467,6 +498,9 @@
>                  sh_video->num_buffered_pts = delay;
>          }
>      }
> +    
> +    if (print_info_on_next_decode) print_decode_info(mpi);

Break this line.

> --- libmpcodecs/dec_video.h	(revision 32249)
> +++ libmpcodecs/dec_video.h	(working copy)
> @@ -22,6 +22,7 @@
>  #include "libmpdemux/stheader.h"
>  
>  extern int field_dominance;
> +extern int print_info_on_next_decode;

This is not the right place for this extern declaration -
the variable is not declared in dec_video.c.

> --- mplayer.c	(revision 32249)
> +++ mplayer.c	(working copy)
> @@ -3815,6 +3815,7 @@
>  if(mpctx->loop_times==1) mpctx->loop_times = -1;
>  
>  mp_msg(MSGT_CPLAYER,MSGL_INFO,MSGTR_StartPlaying);
> +print_info_on_next_decode = 1;
>  
> --- mencoder.c	(revision 32249)
> +++ mencoder.c	(working copy)
> @@ -686,6 +686,7 @@
>    m_config_push(mconfig);
>    m_entry_set_options(mconfig,&filelist[curfile]);
>    filename = filelist[curfile].name;
> +  print_info_on_next_decode = 1;

At a glance these look like pretty random places.

Diego