Skip to content

Loading…

updated libdvdcss, read and nav to the latest versions #2128

Merged
merged 2 commits into from

9 participants

@Paxxi
Team Kodi member

I'm not sure about the etiquette about this, I saw PR 2048 doing part of this but not sure how to ammend his PR.

All XBMC modifications kept intact, even the old xbox stuff.

Built and tested in Win32, tested with/without css and from dvd/image

@davilla

"This pull request cannot be automatically merged." :)

@Paxxi
Team Kodi member

oh crap, spent more time with git than merging trying to get a clean PR. Back to the git fighting then :)

@jmarshallnz
Team Kodi member

git pull --rebase upstream/master
git push -f

@Paxxi
Team Kodi member

Thanks jmarshall, commit should be OK now. I noticed though that there's copies of the libdvdnav header files in xbmc/cores/dvdplayer/DVDInputStream/dvdnav/. I guess these should be updated as well to be kept in sync, or would it be better to just get rid of them and use the headers in lib/libdvd/libdvdnav/src/dvdnav

@elupus
Team Kodi member
@MartijnKaijser
Team Kodi member

Can't the old xbox code be removed as well in an additional commit?

@davilla

You have to be very careful in what is removed here. The changes are not obvious.

@Paxxi
Team Kodi member

Updated the files in core/dvdplayer as well. Removed the big endian defines, data is already read in big endian format and then byteswapped.

@Voyager1 Voyager1 commented on an outdated diff
lib/libdvd/libdvdnav/src/dvdnav_internal.h
@@ -186,18 +175,6 @@ struct dvdnav_s {
/* converts a dvd_time_t to PTS ticks */
int64_t dvdnav_convert_time(dvd_time_t *time);
-/*
- * Get current playback state
- */
-dvdnav_status_t dvdnav_get_state(dvdnav_t *this, dvd_state_t *save_state);
-
-/*
- * Resume playback state
- */
-dvdnav_status_t dvdnav_set_state(dvdnav_t *this, dvd_state_t *save_state);
-
@Voyager1 Team Kodi member
Voyager1 added a note

why are get/setstate being removed here?

@Paxxi Team Kodi member
Paxxi added a note

That seems to be a mistake, going to re-add them but found some oddness along the way

@Voyager1 Team Kodi member
Voyager1 added a note

I was just asking - it compiles fine without. Could it be related to the added functions in xbmc/cores/dvdplayer/DVDInputStreams/dvdnav/vm.h (vm_get_state / vm_set_state) ? I'm not sure.

@Paxxi Team Kodi member
Paxxi added a note

dvdnav_set/get_state was removed from dvd_internals.h but this didn't affect the build of the library because the actual functions were still there. Then because dvdnav_get/set_state were declared in core/dvdplayer../vm.h XBMC found the functions and the build succeeded. It works but for maintenance it's quite a horrible situation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Voyager1 Voyager1 commented on the diff
lib/libdvd/libdvdnav/src/vm/vm.c
@@ -339,16 +369,6 @@ int vm_reset(vm_t *vm, const char *dvdroot) {
fprintf(MSG_OUT, "libdvdnav: vm: failed to open/read the DVD\n");
return 0;
}
-#ifdef _XBMC
- if(DVDUDFVolumeInfo(vm->dvd, vm->dvd_name, sizeof(vm->dvd_name), NULL, 0))
- if(DVDISOVolumeInfo(vm->dvd, vm->dvd_name, sizeof(vm->dvd_name), NULL, 0))
- strcpy(vm->dvd_name, "");
-
- fprintf(MSG_OUT, "libdvdnav: vm: DVD Title: %s\n", vm->dvd_name);
-#else
- dvd_read_name(vm->dvd_name, dvdroot);
-#endif
- vm->map = remap_loadmap(vm->dvd_name);
@Voyager1 Team Kodi member
Voyager1 added a note

why are we getting rid of the _XBMC ifdef here? I'm not sure what the original intent was of this... it seems it's used to populate vm->dvd_name, so is this patch no longer needed?

@Voyager1 Team Kodi member
Voyager1 added a note

disregard - just saw that it has been moved down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Voyager1
Team Kodi member

First of all thanks Paxxi for taking on the effort to update to latest libdvd. I have extensively tested this work with several DVD ISOs (with menus etc.) as well as physical DVDs, and am pleased with the result. Some discs/isos that didn't entirely play correctly before, now do!

We need to have this tested on the other platforms too, otherwise for Windows this is good to go!

@ace20022
Team Kodi member

Big thanks to @Paxxi! This seems to fix a huge bug in get_audio_info, now all relevant info can be queried at once again.

@Paxxi
Team Kodi member

Fixed the things mentioned by @ace20022 and @Voyager1 .

Found that dvdnav_get_state and dvdnav_set_state were declared in vm.h in the includes copied to dvdplayer, moved them to dvdnav_internal.h to match where they are in libdvd folder.

@Paxxi
Team Kodi member

Hopefully that should be the last fix

@Voyager1
Team Kodi member

minor mistake, causing compiler error:
xbmc\xbmc\cores\dvdplayer\dvdinputstreams\dvdnav\dvdnav_internal.h(196): error C2143: syntax error : missing ';' before 'this'
xbmc\xbmc\cores\dvdplayer\dvdinputstreams\dvdnav\dvdnav_internal.h(201): error C2143: syntax error : missing ')' before 'this'

you can't use the reserved keyword "this" here since it's a C++ compiler :-)

I suggest you use "self" like in vm.h where you moved it from.

@Voyager1
Team Kodi member

after that, I suggest that @Memphiz and @arnova (or other platform devs) test it out under IOS/OSX/ATV2 and Linux in order sign this off to be merged.

@MartijnKaijser
Team Kodi member

Let's throw it through Buildbot too and see if it compiles on all platforms. ios needs some special handling first?

@davilla

Should be fine unless the existing stuff in configure.in/makefiles was removed. buildbot will show if there's a problem.

@Memphiz
Team Kodi member

Stop - don't inject yet! There is something wrong ...

@davilla

buildbot can build from a user's github branch :)

@Memphiz
Team Kodi member

This patch is needed for getting it compiled for osx/ios (was lost somehow?)

http://pastebin.com/vKeyJ4EE

The win32 guard is not sufficient cause WIN32 gets "force" defined in

https://github.com/xbmc/xbmc/blob/master/xbmc/cores/dvdplayer/DVDInputStreams/DllDvdNav.h#L29

@Paxxi
Team Kodi member

new commit up, actually made sure it compiles this time

@Memphiz
Team Kodi member

don't use self - its a keyword in objc (like this in c++) and gets highlighted in xcode ... use "me" or something. (see my patch)

@Paxxi
Team Kodi member

@Memphiz just saw your patch, didn't know about objc. Anyway it's bedtime so I'll go over this tomorrow and make the needed changes

@Memphiz
Team Kodi member

After applying my patch it still plays a dvd iso from my nfs nas - so yay fine i guess :D

@davilla

use TARGET_xxxx in XBMC code, LINUX, WIN32, __APPLE_ should not be used.

@Voyager1
Team Kodi member

@Memphiz it's weird what you say about self. Current codebase has it like this... (see xbmc/cores/dvdplayer/DVDInputStreams/dvdnav/vm.h) ... but me's good too :-)

edit: it took a while for me to get it (it's late!) - you're concerned about the syntax highlighting in xcode.

@Voyager1
Team Kodi member

@davilla - agree with the TARGET_xxx for the copied includes under xbmc, but certainly not in libdvd code (compiled by mingw on win), right?

@Memphiz
Team Kodi member

I'm just saying that "self" is highlighted in xcode which looks confusing. (saw it when it bombed out cause of the "this"). This is no compile issue of course but thought i mention it for preventing its usage somwhere deeper in our code in the future ;)

@davilla

@Voyager1 , you have to watch out of reserved keywords in any headers that might be included into obj-c code (extensions of .m or .mm), examples are 'id' and 'self'. When you view non-obj-c code in Xcode, those reserved keywords are highlighted like reserved keywords in c/c++ ( 'for', 'while', or 'if' ).

@Paxxi Paxxi Updated libdvdcss, read and nav to the latest versions
All XBMC modifications are kept intact

Moved get/set state functions from vm.h to dvdnav_internal.h where
they should be

Added comments to clarify which functions are added by XBMC
421d9fd
@Paxxi
Team Kodi member

fixed the _LINUX ifdef, re-added it as it were before instead of trying to change it to TARGET_xxx and how it would affect the forced win32 define.

Left "self" as is since it's used fairly often in the dvdnav sources and it doesn't cause any errors

@Voyager1
Team Kodi member

compiles fine on Windows, although I saw you didn't honor davilla's request to change the platform ifdefs (I see there may be a breakage risk in changing this). Regarding the self var I see the point as the other code is full of it. I hope everyone can settle on this and give the green light to merge it (perhaps have the buildbot run through it first?)

@elupus
Team Kodi member
@Memphiz
Team Kodi member

Now its fine on osx/ios compile wise (and quick runtimetested).

@elupus
Team Kodi member

So the changes for multi angle stuff are still in. What is this updated based on thou? There apparently just showed up another fork for libdvdnav: http://git.videolan.org/?p=libdvdread.git;a=shortlog

http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2013-February/001874.html

@elupus
Team Kodi member

Also I would also be very interested in seeing a diff against the source. Ie what patches we still add.

@Voyager1
Team Kodi member

my assumption was this is based off http://dvdnav.mplayerhq.hu/ 4.2.0

@Paxxi
Team Kodi member

Voyager1 is correct, it's based on http://dvdnav.mplayerhq.hu/ . I didn't know about the new fork.

@elupus I'll fix you a diff tonight of the XBMC changes that are still in

@Paxxi
Team Kodi member

@elupus Here's the diffs of the changes. I've split them up per lib to make it easier to check. cores.diff is the difference between the header files in /lib and /cores/dvdplayer

https://dl.dropbox.com/u/55118895/libdvdread.diff
https://dl.dropbox.com/u/55118895/libdvdnav.diff
https://dl.dropbox.com/u/55118895/libdvdcss.diff
https://dl.dropbox.com/u/55118895/cores.diff

Looking at the new fork over at videolan it probably shouldn't be that much work to update against that once these changes are in. Not sure if there's any immediate need though since the fork seem to be very new. With that said I'm more than willing to do the work if you think it would be better.

@Voyager1
Team Kodi member

Thanks Paxxi. Very useful to have these diffs. Out of curiosity, all the changes/additions in libdvdnav, more specifically in searching.c and vm.c, that are not marked as "xbmc" additions, were these also xbmc changes/additions at the time of 4.1.3, and are these still needed?

@Paxxi
Team Kodi member

Those sections weren't marked as XBMC changes but they're not in the libdvdnav 4.1.3 sources. The best I've found for them is this checkin from @elupus . It would probably make sense to add some _XBMC ifdefs around them to make it easier to track in the future.

I'm not entirely sure wether they're still needed or not.

@Voyager1
Team Kodi member

Thanks for the detective work... eventually we will need to cleanly split up these changes into individual patches that make more sense (with some description) and that we can track. But that would require some expert help, and can be put on the back burner, IMO. For now, we could store these diffs under lib/libdvd/patches?

@elupus - as the merge window is closing in 3 days, agree to merge? It would be a good opportunity to get a bit larger testing base.

@davilla

store these diffs under lib/libdvd/patches, that way like with ffmpeg, we can track.
@elupus , ready for inject ?

@elupus
Team Kodi member

I suppose so.. It's based on something rather old thou. 4.2.0 was released at 10 Oct 2011.

@davilla

well, I think that's the point of the new fork, nothing is happening at the old home. 4.2.0 was the last release.

@Voyager1
Team Kodi member

@Paxxi - happy to merge when you've added the diffs as a second commit.

@Paxxi Paxxi Added diffs for XBMC changes to libdvd libs
cores.diff is the differences between include files in lib/libdvd
and xbmc/cores/dvdplayer/dvdinputstreams/dvdnav
57e27af
@Voyager1 Voyager1 merged commit 6a384a1 into xbmc:master
@ralob

This breaks Linux compile: http://xbmclogs.com/show.php?id=34202
Should I do a Trac ticket?

@ace20022
Team Kodi member

No, see PR #2198 .

@ralob

That PR fixed it right up; sorry about the noise.

@Paxxi Paxxi deleted the Paxxi:libdvd branch
@Voyager1
Team Kodi member

We're getting a bug report (see http://trac.xbmc.org/ticket/14115) on Linux. Can anyone test on Linux and see if this is legit?

@Memphiz
Team Kodi member

No problems playing a dvd iso on linux. (ubuntu 12.10).

Though from the ticket its not clear if it is dvd, or folder structure...

@Voyager1

@elupus @paxxi - I'm currently porting libdvdnav and libdvdread 4.2.1 to a development branch. I have a question regarding the two tests above in vm.c (function ifoOpenNewVTSI) which have been part of our xbmc added patches (already back in 4.1.3): if(!ifoRead_VTS_TMAPT(vm->vtsi)) and if(!ifoRead_TITLE_C_ADT(vm->vtsi)) - any idea why these were added? I'm trying to eliminate a couple of reasons why certain DVDs apparently play correctly with vanilla libdvd 4.2.x but not with our patched version...

Team Kodi member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 5, 2013
  1. @Paxxi

    Updated libdvdcss, read and nav to the latest versions

    Paxxi committed
    All XBMC modifications are kept intact
    
    Moved get/set state functions from vm.h to dvdnav_internal.h where
    they should be
    
    Added comments to clarify which functions are added by XBMC
Commits on Feb 8, 2013
  1. @Paxxi

    Added diffs for XBMC changes to libdvd libs

    Paxxi committed
    cores.diff is the differences between include files in lib/libdvd
    and xbmc/cores/dvdplayer/dvdinputstreams/dvdnav
Something went wrong with that request. Please try again.