[MPlayer-dev-eng] [PATCH] dvdnav part 2 - still frames
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Sun Jul 29 20:50:28 CEST 2007
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.
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.
> + 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.
> 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.
> + if(nextstill) {
> + if(nextstill==0xff) priv->duration=0; else priv->duration=nextstill * 1000;
> + priv->still_length=nextstill;
> + }
closing } is not aligned properly.
> break;
> + }
same here
> + case DVDNAV_AUDIO_STREAM_CHANGE: {
> + priv->status.stream_change = 1;
> + priv->status.audio_change = 1;
> + break;
> + }
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.
> + 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.
> @@ -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.
> 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 {})
> @@ -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.
> @@ -401,9 +470,8 @@
> stream->type = STREAMTYPE_DVDNAV;
> stream->priv=(void*)priv;
> *file_format = DEMUXER_TYPE_MPEG_PS;
> -
> update_title_len(stream);
cosmetics.
> @@ -627,12 +696,22 @@
> return n;
> }
>
> +/**
> + * \brief mp_dvdnav_get_spu_clut() returns the spu clut
> + * \param stream: - stream pointer
> + * \return spu clut pointer
> + */
> unsigned int *mp_dvdnav_get_spu_clut(stream_t *stream) {
> dvdnav_priv_t *priv=(dvdnav_priv_t*)stream->priv;
> if(!priv->spu_set) return NULL;
> return priv->spu_clut;
> }
>
> +/**
> + * \brief mp_dvdnav_get_highlight() get dvdnav highlight struct
> + * \param stream: - stream pointer
> + * \param hl : - highlight struct pointer
> + */
I'll apply these in a few moments.
> +/**
> + * \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.
> + 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.
> 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 ;-)
> 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.
> 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.
[...]
> @@ -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?
> +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.
> + 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?
> @@ -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.
> @@ -3404,7 +3575,6 @@
> time_frame += frame_time / playback_speed; // for nosound
> }
> }
> -
> // ==========================================================================
>
> // current_module="draw_osd";
Cosmetics.
> 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.
Greetings,
Reimar Döffinger
More information about the MPlayer-dev-eng
mailing list