Re: [Dvd-devel] Forking of libdvdread

Håkan Hjort wrote:

>Wed May 07 2003, Michael Roitzsch wrote:
>  
>
>>Hi Hkan,
>>
>>James seems to be away, so I will tell you my opinion, since I will also 
>>be away until Monday.
>>
>>    
>>
>>>I'm sad to have to hear about this.
>>>      
>>>
>>I did not like the idea either.
>>
>>    
>>
>Glad that I'm not alone on this.
>
>
>  
>
>>>Why do you feel it's necessary to fork this code?
>>>
>>>From the cvs commit:
>>>      
>>>
>>>>Add files from libdvdread.
>>>>This is to provide better integration with libdvdnav.
>>>>Better error messages reported to the application etc.
>>>>        
>>>>
>>>What do you feel is missing?  Adding better return codes and a
>>>structured error message system would be a good things in general and
>>>not just some thing that only libdvdnav would benefit from.
>>>      
>>>
>>Heavily agreed.
>>
>>    
>>
>>>>We will modify libdvdread files to be portable across all
>>>>compilers. Currently, libdvdread only behaves with gcc.
>>>>        
>>>>
>>>It behaves with many versions of gcc and Suns Forte C compiler.
>>>These are the two compilers I have access to and care for.
>>>
>>>If you want to make it more portable be my guest, I'll take those
>>>patches. No need to create a fork for that.  I just won't do this
>>>work my self because it already work on all platforms and compilers
>>>I have tested with.
>>>      
>>>
>>From what James explained to me:
>>He thinks the bitfield structures are very insecure concerning 
>>portability, since compilers are known to "optimize" such packed data 
>>and align it in some way. I agree with that.
>>IIRC you already had such problems with some Sun compiler and fixed some 
>>in a rather hacky way by changing the order of one of the structures to 
>>one the compiler does handle.
>>James thinks that a portable way would be to use a getbits() method to 
>>read the data from the disc and store it in the structures. I think he 
>>is right here. He said he discussed that with the libdvdread team 
>>(you?), but they denied that need.
>>
>>    
>>
>I simply said that _I_ would not be making those changes.  I have
>no problem with patches making libdvdread more portable, though 
>this would again break the ABI.
>
>More specifically to 'getbits()', I feel that that should not be
>necessary, the IFO and NAV structures are almost exlucivly byte /
>short / word length data and at fixed offsets from some point that can
>be computed.  So I'd think it would be better to make it byte based,
>and only in those few cases where there are bit based fileds handle
>those with a bits based macro/function.  Though this is just details.
>

If you care to look at my getbits() function, you will in fact see that 
it is dual bit/byte ready.
I am using the getbits() function to parse the binary packed NAV and IFO 
structures into C structures.
This parsing only happens once in a while, so performance is certainly 
not the issue here.
I prefer nicer looking code in this case.
It would be easy to add inline get16bits() and get32bits() functions to 
the getbits() api that contain no "if" statements and the resulting 
machine code would be as fast as any other code to do the job. I did the 
current getbits() code for parsing NAV packs in a very short time(1 hour 
or less I think), and just so we could have nice looking code on MSVC. 
The only other way around the bit packing problem appeared to be some 
not-so-neat macros to hide memory/structure manipulations.

>
>
>  
>
>>>>libdvdread does not like Microsoft Compilers.
>>>>        
>>>>
>>>Yes, so why do you need to use them?  There are the mingwin and
>>>cygwin systems for Win32 platforms.  Granted I've not tested with
>>>them either.
>>>      
>>>
>>That's not the point here. MSVC is not a very good compiler, but a very 
>>widely spread one. libdvdread should compile with it if open-source 
>>DVD-playback wants to conquer the Windows platform.
>>
>>    
>>
>Oh well.. I still feel that mingwin could be used instead
>(if that isn't broken too).
>
The current way libdvdread does parsing or mapping from ifo and nav is 
the wrong way to do it.
Packing of bit field structs in the C language will vary from compiler 
to compiler and even from compiler versions to other versions, so to 
have truely portable C code, one cannot assume any particular packing 
format. It was this issue that I was striving to fix and I believe I 
have managed that with some fairly neat looking code. Currently, only 
nav parsing and header files have changed.

>>>>This portability modification will change libdvdread so much,
>>>>that I thought it best to finally combine libdvdnav and libdvdread
>>>>into one lib.
>>>>        
>>>>
>>>Why would it need to do that?
>>>The types in ifo_types.h, nav_types.h would be simplified but not
>>>necessary change names or even break source compatibly.
>>>      
>>>
>>That's right. The headers did not change dramatically. (some not at all)
>>
They have not changed yet. I have only had 1-2 hours work so far on the 
changes. Many more are likely to happen.

>>>The functions in ifo_read.c and nav_read.c would have to be updated
>>>to match the new types but that would practically be that.
>>>      
>>>
>>James has already done quite some work on the getbits(). It should be 
>>fairly easy to extract the patches out of cvs. Maybe you can have a 
>>look at them and see, if there is any way this could be merged back 
>>into libdvdread. If that would be so, I would definitely vote for 
>>throwing the code off libdvdnav again.
>>
>>    
>>
>Sure, though from what I've seen in the web cvs, there have been lots 
>of (IMHO) pointless and/or wrong changes to the files which will make 
>merging harder (some of the win32 changes).  Perhaps the types/reader
>files are in better shape, I've yet to look at those.
>
>  
>
Maybe I should try to explain some of the reasons for doing this.
1) I wanted to get libdvdnav and associated tools to compile on any 
platform. I believe the getbits() and possibly adding get16/32bits() 
achieve this.
2) I wanted to experiment with different methods of error reporting.
  a) The application to get knowledge if the DVD required 
authentication/ diagnose errors. E.g. DVD drive region locking etc.
  b) The application to get knowledge if the DVD requires CSS support.
  c) The application to handle errors on the DVD sector reads.
3) Lots of other projects currently use libdvdread, and any changes we 
might make to the api would take a long time to reach the user. A lot of 
package handlers do not like 2 versions of the same lib being loaded, so 
upgrading libdvdread on the user's platform would probably break other 
applications.
4) libdvdnav is still very much a work in progress, particularly with 
regard to the api, so making changes to the current libdvdread 
functionallity, if needed, could be made in the libdvdnav tree without 
effecting libdvdread in anyway.
5) I could not really see why 2 different libs were really technically 
needed. Having one lib seemed to me to be neater.
6) libdvdread changes very rarely, so syncing libdvdread changes into 
libdvdnav will not be a big problem.
7) I see this more of a re-use of some source code files, rather than a 
fork.
8) The videolan project have their own dvd lib, libdvdplay, and I think 
that will be doing it's own dvd reading, and ifo/nav parsing. I can't 
really understand their need for it, when libdvdnav is already here, but 
they fail to answer any emails, so I don't really know why they have 
embarked on it.

Cheers
James