[MPlayer-dev-eng] [PATCH] dvdnav - partial REVIEW

[Nico Sabbi]

Ötvös Attila wrote:

>Hi All
>
>I implemented the activation event of the menu button showing (change color) 
>and updated with CVS.
>
>I created patch (navdoc.patch) to mplayer man (en) but please check! (I am 
>sorry to say that my English knowledge isn't good)
>
>I want to known if I should do something.
>
>http://dcxx.fw.hu/
>
>Best regards!
>Attila
>  
>
Hi,
I reviewed the parts of your dvdnav patch relative to the stream and mux 
layer.

First of all: you added a directory with a scary name -libmpdvdnav-, 
that contains
a modified version of libdvdnav. IMO this not acceptable; I wish that 
you could
make your code work with the standard libdvdnav (importing still another 
projtect
in mplayer is a serious problem: the sources tarball would grow even more
and no one would fix/maintain it).

Second: you added a second dvdnav stream. This is wrong, because there's
no reason to keep the old one lying around unused. You should patch the 
old one instead.

Consequently all the blocks of code #ifdef-ed MPDVDNAV should not exist 
anymore.

Attached are two patches that you should take as starting point: 
configure2.diff
is self-explaining, demux2.diff is what is left of your changes in 
libmpdemux/ after I removed
what I consider unacceptable.
Let's see part of it:


Index: demux_mpg.c
===================================================================
RCS file: /cvsroot/mplayer/main/libmpdemux/demux_mpg.c,v
retrieving revision 1.82
diff -u -r1.82 demux_mpg.c
--- demux_mpg.c 3 May 2006 18:18:25 -0000       1.82
+++ demux_mpg.c 6 May 2006 16:14:47 -0000
@@ -754,6 +754,7 @@
          int i;
           if(sh_audio && !d_audio->eof && d_video->pts && d_audio->pts){
            float a_pts=d_audio->pts;
+         if((float)sh_audio->i_bps==0.0f) break;


why this ??

             
a_pts+=(ds_tell_pts(d_audio)-sh_audio->a_in_buffer_len)/(float)sh_audio->i_bps;
            if(d_video->pts>a_pts){
              skip_audio_frame(sh_audio);  // sync audio
@@ -860,6 +861,24 @@
             *((int*)arg) = demuxer->audio->id;
             return DEMUXER_CTRL_OK;

+       case DEMUXER_CTRL_SET_TIME_PTS: {
+             if (demuxer->movi_end==0)
+               {
+               mpg_d->final_pts = 0.0;
+               mpg_d->has_valid_timestamps = 0;
+               return DEMUXER_CTRL_OK;
+               }
+              mpg_d->final_pts=*((float *)arg);
+             if (mpg_d->final_pts==0.0)
+               {
+               mpg_d->final_pts = 0.0;
+               mpg_d->has_valid_timestamps = 0;
+               return DEMUXER_CTRL_OK;
+               }
+             mpg_d->has_valid_timestamps = 1;
+             return DEMUXER_CTRL_OK;
+             }
+
        default:
            return DEMUXER_CTRL_NOTIMPL;
     }


why do you need to reset the timers and the position? when you change 
the titleset?
If so, you should reset the stream and reopen the demuxer, IMO

-----------------------------------------------------------------------------------------------------------

Index: demuxer.c
===================================================================
RCS file: /cvsroot/mplayer/main/libmpdemux/demuxer.c,v
retrieving revision 1.222
diff -u -r1.222 demuxer.c
--- demuxer.c   27 Apr 2006 11:13:21 -0000      1.222
+++ demuxer.c   6 May 2006 16:14:47 -0000
@@ -253,6 +253,13 @@
         if (!demux_aid_vid_mismatch)
           mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_ID=%d\n", id);
     }
+#ifdef USE_MPDVDNAV
+  ((sh_video_t*)demuxer->v_streams[id])->config_lock=0;
+  ((sh_video_t*)demuxer->v_streams[id])->config_w=0;
+  ((sh_video_t*)demuxer->v_streams[id])->config_h=0;
+  ((sh_video_t*)demuxer->v_streams[id])->config_outfmt=0;
+  ((sh_video_t*)demuxer->v_streams[id])->enable_mpeg2_reset=0;
+#endif
     return demuxer->v_streams[id];
 }

let alone that MPDVDNAV shouldn't even exist, code like that should never
go in the demuxer layer - unacceptable IMO

-----------------------------------------------------------------------------------------------------------

in demuxer.h:
// --------------
+  int max_video_packs;
+  int max_video_bytes;
+  int max_audio_packs;
+  int max_audio_bytes;
+  int hide_packs_err_mess;
// --------------
[...]
} demuxer_t;

the above members that you want to add are completely useless, IMO.
Why are you so scared of the message ("too many packets...") that comes 
out?
Adding a DEMUXER_CTRL to silence it is overkill and in any case you can
fix this issue in a second moment.
Another thing that I really don't like is the code to manage input 
events in the stream layer
(stream_dvdnav.c), but since libdvdnav seems to work only with events 
(and for the
sake of self-conteinance) this is a secondary problem for the time being.


Generally, you should avoid to mix decoder-specific code in the demuxer 
layer, and try to
minimize the impact of your patches on the source.

In my opinion you should begin to send a very minimalistic set of 
patches just to make
the basic functionalities work (i.e. beginning with configure and with 
the necessary changes
in the stream layer) and add functionalities on top of them only when 
the previous round
of patches is applied.

Reviewing your code is very difficult because it's huge and full of 
cosmetics, but I'm sure
you can make it smaller and still have the same functionalities working.

Keep up the good work!

    Nico




-------------- next part --------------
A non-text attachment was scrubbed...
Name: configure2.diff
Type: text/x-patch
Size: 4649 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20060506/a30db852/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: demux2.diff
Type: text/x-patch
Size: 5090 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20060506/a30db852/attachment-0001.bin>