[MPlayer-dev-eng] [PATCH] DVDNAV Still frames supprt

[Reimar Döffinger]

Hello,
On Thu, Jan 24, 2008 at 11:29:28PM +0100, Benjamin Zores wrote:
> +/* state flags */
> +typedef enum {
> +  NAV_FLAG_EOF                  = 0x0001,  /* end of stream has been reached */
> +  NAV_FLAG_WAIT                 = 0x0002,  /* wait event */
> +  NAV_FLAG_WAIT_SKIP            = 0x0004,  /* wait skip disable */
> +  NAV_FLAG_CELL_CHANGED         = 0x0008,  /* cell change event */
> +  NAV_FLAG_WAIT_READ_AUTO       = 0x0010,  /* wait read auto mode */
> +  NAV_FLAG_WAIT_READ            = 0x0020,  /* suspend read from stream */
> +  NAV_FLAG_VTS_DOMAIN           = 0x0040,  /* vts domain */
> +} dvdnav_state_t;

1 << 0
to
1 << 6
would be nicer IMO.

> +        /* set still frame duration */
> +        priv->duration =
> +          (priv->still_length != 255) ? priv->still_length * 1000 : 0;

This kind of conversions is done at least three times (once in the form
of if (...== 255) ).
IMO make a static inline still_duration function or something like this
for this conversion.

> +        if (priv->still_length <= 1) {
> +          pci_t *pnavpci = dvdnav_get_current_nav_pci (priv->dvdnav);
> +          priv->duration = mp_dvdtimetomsec (&(pnavpci->pci_gi.e_eltm));

Innermost () are not necessary

> +int mp_dvdnav_skip_still (stream_t *stream) {
> +  dvdnav_priv_t *priv = (dvdnav_priv_t *) stream->priv;

IIRC priv is void *, so all these casts are not necessary (there are at
least 4).

> +  if (priv->still_length == 0xff)

This is the "if" I meant in the first comment.

> +  if (clear) {
> +    priv->state &= ~NAV_FLAG_CELL_CHANGED;
> +  }

Useless {}

> +#ifdef USE_DVDNAV
> +/* store decoded video image */
> +static mp_image_t * mp_dvdnav_copympi(mp_image_t* to_mpi,
> +                                      mp_image_t* from_mpi) {

Not a doxygen comment.

> +    mp_image_t *mpi;
> +
> +    if (!from_mpi)
> +      return NULL;

Is this case _supposed_ to happen?

> +    /* Do not store B-frames */
> +    if (from_mpi->pict_type == 3)

Hmm... are there no constants/defines for pict_type?

> +      return to_mpi;
> +
> +    if (to_mpi &&
> +        to_mpi->w == from_mpi->w &&
> +        to_mpi->h == from_mpi->h &&
> +        to_mpi->imgfmt == from_mpi->imgfmt)
> +      mpi = to_mpi;
> +    else {
> +        if (to_mpi)
> +          free_mp_image(to_mpi);

Indentation is only 2 elsewhere.

> +/* video frame pre-processing: will be called before decode_video() */
> +static void * mp_dvdnav_decode_video_pre(int *in_size,
> +                                         unsigned char **start,
> +                                         mp_image_t* decoded_frame) {

Not doxygen-compatible either.

> +    /* change in dvdnav stream */
> +    if (mp_dvdnav_cell_has_changed(mpctx->stream,0)) {
> +      /* suspend read from dvdnav stream, and set auto_wait */
> +        mp_dvdnav_read_wait(mpctx->stream, 1, 1);
> +        /* free the last stored video frame */
> +        if (mpctx->smpi) free_mp_image(mpctx->smpi);
> +        mpctx->smpi = NULL;
> +        if (mpctx->last_buffer) free(mpctx->last_buffer);
> +        mpctx->last_buffer = NULL;
> +        mpctx->last_in_size = 0;
> +        if (mpctx->d_sub) dvdsub_id = -2;
> +
> +        if (mpctx->sh_video) {
> +            /* clear video pts */
> +            mpctx->d_video->pts                 = 0.0f;
> +            mpctx->sh_video->pts                = 0.0f;
> +            mpctx->sh_video->i_pts              = 0.0f;
> +            mpctx->sh_video->last_pts           = 0.0f;
> +            mpctx->sh_video->num_buffered_pts   = 0;
> +            mpctx->sh_video->num_frames         = 0;
> +            mpctx->sh_video->num_frames_decoded = 0;
> +            mpctx->sh_video->timer              = 0.0f;
> +            mpctx->sh_video->stream_delay       = 0.0f;
> +            mpctx->demuxer->stream_pts          = MP_NOPTS_VALUE;
> +            mpctx->sh_video->timer              = 0;
> +        }
> +
> +        if (mpctx->sh_audio) {
> +            /* free audio packets and reset */
> +            ds_free_packs(mpctx->d_audio);
> +            audio_delay -= mpctx->sh_audio->stream_delay;
> +            mpctx->delay =- audio_delay;

That - is placed weirdly. I am also not too happy that this code messes
directly with mplayer.c "internals", but that may be hard to avoid.

> +    mpctx->dup_frame = 0;
> +    if (*in_size < 0 && mpctx->last_buffer) {
> +        mpctx->dup_frame = 1;

Hmm. how about
> mpctx->dup_frame = *in_size < 0 && mpctx->last_buffer;
> if (mpctx->dup_frame)
?
I think this conveys the meaning of the code better, but I may have
misunderstood the code.

> +        if (!mpctx->libmpeg2_count)
> +            mpctx->libmpeg2_count = 5;
> +
> +        if (mpctx->last_start) {
> +            *start = mpctx->last_start;
> +            *in_size = mpctx->last_in_size;
> +            memcpy(*start,mpctx->last_buffer,*in_size);
> +        } else {
> +            *start = mpctx->last_buffer;
> +            *in_size = mpctx->last_in_size;
> +        }

in_size is set to the same thing in both cases, it might make sense
to pull it out of the if.
Actually I think that
> memcpy(*start,mpctx->last_buffer,mpctx->last_in_size);
might make more sense (and allows setting in_size below the if which I
think would result in the most readable code - YMMV though, do as you
find suitable).


> +        if ((mpctx->sh_video->pts >= len || !mpctx->smpi) &&
> +            (mpctx->sh_video->pts > 0.0) && (len > 0.0)) {

Useless () around the comparisons.

> +                    mp_msg(MSGT_CPLAYER,MSGL_ERR,
> +                        "Can't decode still frame.\n"
> +                        "Please, you try play dvdnav to -vc ffmpeg12 option.\n");

That's not quite English.

> +            if (mpctx->last_buffer)
> +                memcpy(mpctx->last_buffer,start,in_size);
> +            else
> +                mpctx->last_in_size=-1;

I'd have to do a second review pass to be sure, but I have some doubts
the mpctx->last_in_size==-1 case is handled correctly everywhere.
What does it signify anyway compared to 0 (this probably should be
documented at the declaration)?

> +#ifdef USE_DVDNAV
> +	if (mpctx->stream->type == STREAMTYPE_DVDNAV)
> +	    decoded_frame = mp_dvdnav_decode_video_pre(&in_size,
> +                                                       &start,decoded_frame);
> +	/* if image has been stored, there's no need to call decode_video() */
> +	if (in_size > 0 && !decoded_frame)
> +#endif
>  	current_module = "decode_video";
>  	decoded_frame = decode_video(sh_video, start, in_size, drop_frame,
>  				     sh_video->pts);

Something looks fishy to me here.

> -	current_module = "filter_video";
> +#ifdef USE_DVDNAV
> +	if (mpctx->stream->type == STREAMTYPE_DVDNAV)
> +	    decoded_frame = mp_dvdnav_decode_video_post(in_size,
> +                                                        start,decoded_frame,
> +                                                        sh_video, drop_frame);
> +#endif
> +        current_module = "filter_video";
>  	*blit_frame = (decoded_frame && filter_video(sh_video, decoded_frame,

Surrounding code uses a tab for indentation so this should as well.

> +#ifdef USE_DVDNAV
> +    void *smpi;          ///< store last decoded video image
> +    void *last_buffer;   ///< store last read video frame
> +    void *last_start;    ///< video read buffer ptr
> +    int last_in_size;    ///< last read size
> +    int libmpeg2_count;  ///< libmpeg2 repeat decode counter
> +    int dup_frame;       ///< duplicate frame mode

Have a quick thought if you can improve these, esp. the "last read size"
does not provide any information that variable name does not already.

Note that this is only a very quick review, I have not truly understood
even half of the code...

Greetings,
Reimar Döffinger