[MPlayer-dev-eng] [PATCH] dvdnav part 2 - still frames

[Ötvös Attila]

2007. július 29. 20.50 dátummal Reimar Döffinger ezt írta:
Hello Reimar Döffinger!

Thanks your answer.

> Hello,
>
> On Sun, Jul 29, 2007 at 12:07:21AM +0200, ?tv?s Attila wrote:
> > +//
> > +// convert dvd time to millisec
> > +//
> > +static int dvdtimetomsec(dvd_time_t *dt)
> > +{
> > +  static int framerates[4] = {0, 2500, 0, 2997};
> > +  int framerate = framerates[(dt->frame_u & 0xc0) >> 6];
> > +  int msec = (((dt->hour & 0xf0) >> 3) * 5 + (dt->hour & 0x0f)) *
> > 3600000;
>
> This is awfully obfuscated, and for no good reason. >> 3 * 5 is actually
> most likely slower than >> 4 * 10, and the later means that
> 1) the & 0xf0 is not needed.
> 2) it is obvious that this is just bcd.
>
I corrected.

> And actually IMO you should factor that out into a
>
> > static int bcd2int_8(uint8_t in) {
> >   return (in >> 4) * 10 + (in & 0x0f);
> > }
>
> or similar.
>
> > -          dvdnav_still_skip(priv->dvdnav); // don't let dvdnav stall on
> > this image -
> > +//          dvdnav_still_skip(priv->dvdnav); // don't let dvdnav stall
> > on this image
>
> That is confusing because it is unclear if the comment explains why it
> is there or why it is commented out. Unless you think it will be needed
> again in the future just remove it.
I removed.

>
> > +    priv->still_length=still_event->length;
> > +    if (priv->still_length!=255)    // set still frame duration
> > +        priv->duration = priv->still_length * 1000;
> > +        else
> > +        priv->duration = 0;
> > +        if (priv->still_length==0x01) {     // FIXME: TIME HACK
> > +        pci_t* pnavpci = dvdnav_get_current_nav_pci( priv->dvdnav );
> > +        priv->duration = dvdtimetomsec(&(pnavpci->pci_gi.e_eltm));
> > +        }
>
> The indentation here is a complete mess.
I corrected.

>
> >        case DVDNAV_CELL_CHANGE: {
> > +        priv->status.wait_skip=0;
> > +        priv->status.audio_change = 1;
> > +        priv->status.spu_change = 1;
> > +        priv->status.stream_change = 1;
> >          dvdnav_cell_change_event_t *ev = 
> > (dvdnav_cell_change_event_t*)buf; if(ev->pgc_length)
> >            priv->duration = ev->pgc_length/90;
> > +        priv->status.vts_domain=dvdnav_is_domain_vts(priv->dvdnav);   
> > // is vts +
> > +        uint32_t nextstill = dvdnav_get_next_still_flag(priv->dvdnav);
>
> declarations must be at start of block.
I corrected.

> > +        if(nextstill) {
> > +            if(nextstill==0xff) priv->duration=0; else
> > priv->duration=nextstill * 1000; +           
> > priv->still_length=nextstill;
> > +            }
>
> closing } is not aligned properly.
>
> >          break;
> > +        }
I corrected.

> same here
>
> > +      case DVDNAV_AUDIO_STREAM_CHANGE: {
> > +        priv->status.stream_change = 1;
> > +        priv->status.audio_change = 1;
> > +        break;
> > +        }
I removed.

> and here
>
> > -      case DVDNAV_WAIT:
> > -        dvdnav_wait_skip(priv->dvdnav);
> > +      case DVDNAV_WAIT: {
> > +//        dvdnav_wait_skip(priv->dvdnav);
> > +        // if don't open demuxer then suspend read in dvdnav stream else
> > wait skip +        if(priv->status.wait_skip && !priv->status.wait)
> > +            dvdnav_wait_skip(priv->dvdnav);
> > +            else
> > +            priv->status.wait=1;
> >          break;
> > +        }
>
> weird indentation.
I corrected.

> > +      case DVDNAV_VTS_CHANGE: {
> > +            priv->status.wait_skip=0;
> > +            priv->status.audio_change = 1;
> > +            priv->status.spu_change = 1;
> > +            priv->status.stream_change = 1;
> > +        break;
> > +        }
>
> break is indented weirdly here as well.
I corrected.

> > @@ -255,25 +313,36 @@
> >          return 0;
> >        }
> >        switch (event) {
> > -        case DVDNAV_STOP:
> > +        case DVDNAV_STOP: { priv->status.eofstream=1; return len; }
> >          case DVDNAV_BLOCK_OK:
> >          case DVDNAV_NAV_PACKET:
> >            return len;
> > +        case DVDNAV_STILL_FRAME: return len;
> > +        case DVDNAV_WAIT: { if(priv->status.wait) return len; break; }
>
> There is no need for the {}, but please put the code into new lines.
I corrected.

> >              dvdnav_get_highlight (priv, 0);
> > -            if(priv->title > 0 && tit != priv->title)
> > +            if(priv->title > 0 && tit != priv->title) {
> >                return 0;
> > +            }
>
> Cosmetics? (only adding unneeded {})
I removed.

> > @@ -383,12 +452,12 @@
> >      }
> >      if(dvd_chapter > 0)
> >        dvdnav_part_play(priv->dvdnav, p->track, dvd_chapter);
> > -  } else if(p->track == -1)
> > +  } else if(p->track == 0)
> >      dvdnav_menu_call(priv->dvdnav, DVD_MENU_Root);
> > -  else {
> > +  /*else {      // **FIXME** disable to test still frame with
> > still_length mp_msg(MSGT_OPEN,MSGL_INFO,"dvdnav_stream, you didn't
> > specify a track number (as in dvdnav://1), playing whole disc\n");
> > dvdnav_menu_call(priv->dvdnav, DVD_MENU_Title);
> > -  }
> > +  }*/
>
> IMO better use #if 0 / #endif to comment out even small blocks of code.
I corrected.

> > @@ -401,9 +470,8 @@
> >    stream->type = STREAMTYPE_DVDNAV;
> >    stream->priv=(void*)priv;
> >    *file_format = DEMUXER_TYPE_MPEG_PS;
> > -
> >    update_title_len(stream);
>
> cosmetics.
I removed

> I'll apply these in a few moments.
Thaks.

> > +/**
> > + * \brief mp_dvdnav_isreallyeof() query is really dvd stream eof
> > + * \param stream: - stream pointer
> > + * \return 1 if really eof
> > + */
> > +int mp_dvdnav_isreallyeof(stream_t *stream) {
> > +    dvdnav_priv_t * priv=(dvdnav_priv_t*)stream->priv;
>
> I know, the other code uses it as well, but that (dvdnav_priv_t*) is
> really useless.
I corrected.

> > +    return (priv->status.eofstream==1);
>
> And the () here are not really needed either.
>
> > +    if (mode>=0) priv->status.wait_read=(mode!=0);
> > +    if (automode>=0) priv->status.wait_read_automode=(automode!=0);
>
> IMO write "!mode" and "!automode" instead of != 0.
I corrected.

> >  typedef struct {
> > +  unsigned int     eofstream          : 1;  /* stream eof flag */
> > +  unsigned int     wait               : 1;  /* wait event */
> > +  unsigned int     wait_skip          : 1;  /* wait skip disable */
> > +  unsigned int     cell_change        : 1;  /* cell change event */
> > +  unsigned int     audio_change       : 1;  /* audio change event */
> > +  unsigned int     spu_change         : 1;  /* spu change event */
> > +  unsigned int     wait_read_automode : 1;  /* wait read auto mode (if
> > opening demuxer then off else on) */ +  unsigned int     wait_read       
> >   : 1;  /* wait read flag (suspend read from dvdnav stream */ +  unsigned
> > int     stream_change      : 1;  /* stream change flag: title, part,
> > audio or spu */ +  unsigned int     vts_domain         : 1;  /* is vts */
> > +} nav_status_t;
>
> bitfield crap ;-). I'd take a normal int with flag-defines over that any
> day. But no, I certainly don't expect you to change it, just couldn't
> stand not saying anything ;-)
I replace to bit mask.

>
> >    dvdnav_highlight_event_t hlev;
> > +  int              still_length;        /* still frame duration */
> > +  nav_status_t     status;
> >  } dvdnav_priv_t;
> >
> > -
> >  int dvdnav_number_of_subs(stream_t *stream);
>
> Cosmetics.
I removed.

>
> >  int dvdnav_aid_from_lang(stream_t *stream, unsigned char *language);
> >  int dvdnav_lang_from_aid(stream_t *stream, int id, unsigned char *buf);
> > @@ -38,5 +52,9 @@
> >  void mp_dvdnav_update_mouse_pos(stream_t *stream, int32_t x, int32_t y,
> > int* button); void mp_dvdnav_get_highlight (stream_t *stream,
> > nav_highlight_t *hl); unsigned int *mp_dvdnav_get_spu_clut(stream_t
> > *stream);
> > -
> > +int mp_dvdnav_isreallyeof(stream_t *stream);
> > +int mp_dvdnav_still_skip(stream_t *stream);
> > +int mp_dvdnav_wait_skip(stream_t *stream);
> > +int mp_dvdnav_iscellchage(stream_t *stream, int clear);
> > +void mp_dvdnav_wait_read(stream_t *stream, int mode, int automode);
> >  #endif
>
> And leave that empty line before the #endif there as well.
I corrected.

>
> [...]
>
> > @@ -1014,6 +1028,7 @@
> >    if (vo_spudec==NULL && mpctx->stream->type==STREAMTYPE_DVDNAV) {
> >      unsigned int *palette = mp_dvdnav_get_spu_clut(mpctx->stream);
> >      current_module="spudec_init_dvdnav";
> > +    palette=NULL;
> >      vo_spudec=spudec_new_scaled(palette, mpctx->sh_video->disp_w,
> > mpctx->sh_video->disp_h); }
> >  #endif
>
> unrelated?
I removed.

> > +mp_image_t* mp_dvdnav_copympi(mp_image_t* tompi, mp_image_t* frommpi) {
> > +    mp_image_t* mpi;
> > +
> > +    if(!frommpi) return NULL;
> > +    if(frommpi->pict_type==3) return tompi;     // B-frame don't store
> > +    if(tompi && tompi->w==frommpi->w && tompi->h==frommpi->h &&
> > tompi->imgfmt==frommpi->imgfmt) { +    mpi=tompi;
> > +    } else {
> > +    if(tompi) free_mp_image(tompi);
> > +    if(frommpi->w==0 || frommpi->h==0) return NULL;
> > +    mpi=alloc_mpi(frommpi->w,frommpi->h,frommpi->imgfmt);
> > +    }
>
> horrible indentation.
I corrected.
> > +        if (!mpctx->libmpeg2_count) {               // is first call
> > +            mpctx->libmpeg2_count=5;                // repeat number = 5
> > +            }
>
> useless {}. And placing the } like this seems to be your style, but
> given that none of the other code and no other developer I know of
> places it like this, could you kindly ignore your own preferences in
> this case?
I corrected.

> > @@ -2000,7 +2146,7 @@
> >      *blit_frame = 0; // Don't blit if we hit EOF
> >      if (!correct_pts) {
> >  	unsigned char* start=NULL;
> > -	void *decoded_frame;
> > +    void *decoded_frame=NULL;
>
> Old code used a tab to indent, please leave it that way.
I changed to tab.

> > @@ -3404,7 +3575,6 @@
> >  	  time_frame += frame_time / playback_speed;  // for nosound
> >        }
> >    }
> > -
> >  //
> > =========================================================================
> >=
> >
> >  //    current_module="draw_osd";
>
> Cosmetics.
I removed.

> > Index: command.c
> > ===================================================================
> > --- command.c	(revision 23887)
> > +++ command.c	(working copy)
> > @@ -2655,9 +2655,11 @@
> >
> >  		if (mp_dvdnav_handle_input
> >  		    (mpctx->stream, cmd->args[0].v.i, &button)) {
> > +#if 0
> >  		    uninit_player(INITED_ALL - (INITED_STREAM | INITED_INPUT |
> >  				   (fixed_vo ? INITED_VO : 0)));
> >  		    brk_cmd = 2;
> > +#endif
>
> Hmm.. looks suspicious. Needs at least a comment.
I add to comment.

Best regards.
Attila
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dvdnav-2-still.patch
Type: text/x-diff
Size: 22295 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070730/fbac1c45/attachment.patch>