[PATCH] Add serialization support
Hi, The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string. This is a patch I created for MythTV, where it is currently in use. As I wrote in the submit description for MythTV: "The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle." Richard.
Dear Richard, Am Freitag, den 18.10.2013, 21:05 +0200 schrieb Richard Hulme:
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle."
thank you for your patch. Applying your patch to a git repository, committing it and doing `git show` (with `git config color.ui auto` (default since 1.8.4)), it shows several whitespace errors in `src/libdvdnav/Makefile`. Furthermore, running $ cppcheck --version # from Debian Sid/unstable Cppcheck 1.61 $ cppcheck --enable=all src/vm/vm.c # Cppcheck 1.6 does not show anything regarding your patch. Your usage of {}, if it is put on the same or on the next line, in `src/dvdnav.c` seems inconsistent.
+ /* set the state. this will also start the vm on that state */ + /* means the next read block should be comming from that new */ + /* state */
s/means/meaning/ s/comming/coming/ Thanks, Paul
On 18/10/13 23:20, Paul Menzel wrote:
Dear Richard,
Am Freitag, den 18.10.2013, 21:05 +0200 schrieb Richard Hulme:
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle."
thank you for your patch.
Applying your patch to a git repository, committing it and doing `git show` (with `git config color.ui auto` (default since 1.8.4)), it shows several whitespace errors in `src/libdvdnav/Makefile`.
Hi Paul, As I forgot to add the changes to Makefile to the diff, any whitespace errors there are not caused by me :)
Furthermore, running
$ cppcheck --version # from Debian Sid/unstable Cppcheck 1.61 $ cppcheck --enable=all src/vm/vm.c # Cppcheck 1.6
does not show anything regarding your patch.
Is that good or bad?
Your usage of {}, if it is put on the same or on the next line, in `src/dvdnav.c` seems inconsistent.
My usage of braces is consistent. Other people's usage is sometimes inconsistent with mine, which is unfortunate when I cut'n'paste their code and don't think on to go over it all again :)
+ /* set the state. this will also start the vm on that state */ + /* means the next read block should be comming from that new */ + /* state */
s/means/meaning/ s/comming/coming/
Yep, cut'n'paste. Ok, I've made some changes (added the missing vm_serialize.c to Makefile, fixed formatting of braces, changed C++ style comments to C and resolved some type issues). Hopefully this looks a bit better. Richard.
Dear Richard, Am Sonntag, den 20.10.2013, 19:10 +0200 schrieb Richard Hulme:
On 18/10/13 23:20, Paul Menzel wrote:
Am Freitag, den 18.10.2013, 21:05 +0200 schrieb Richard Hulme:
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle."
thank you for your patch.
Applying your patch to a git repository, committing it and doing `git show` (with `git config color.ui auto` (default since 1.8.4)), it shows several whitespace errors in `src/libdvdnav/Makefile`.
As I forgot to add the changes to Makefile to the diff, any whitespace errors there are not caused by me :)
Ah. So they are from Autotools(?) then I guess.
Furthermore, running
$ cppcheck --version # from Debian Sid/unstable Cppcheck 1.61 $ cppcheck --enable=all src/vm/vm.c # Cppcheck 1.6
does not show anything regarding your patch.
Is that good or bad?
That’s good.
Your usage of {}, if it is put on the same or on the next line, in `src/dvdnav.c` seems inconsistent.
My usage of braces is consistent. Other people's usage is sometimes inconsistent with mine, which is unfortunate when I cut'n'paste their code and don't think on to go over it all again :)
+ /* set the state. this will also start the vm on that state */ + /* means the next read block should be comming from that new */ + /* state */
s/means/meaning/ s/comming/coming/
Yep, cut'n'paste.
Ok, I've made some changes (added the missing vm_serialize.c to Makefile,
I think you need to add it to `src/vm/Makefile.am`. Though that did not get it included into my main `Makefile` after running `./configure`.
fixed formatting of braces, changed C++ style comments to C and resolved some type issues). Hopefully this looks a bit better.
Thanks for doing that! Thanks, Paul
On 21/10/13 00:30, Paul Menzel wrote:
Furthermore, running
$ cppcheck --version # from Debian Sid/unstable Cppcheck 1.61 $ cppcheck --enable=all src/vm/vm.c # Cppcheck 1.6
does not show anything regarding your patch.
Is that good or bad?
That’s good.
That's what I thought :)
Ok, I've made some changes (added the missing vm_serialize.c to Makefile,
I think you need to add it to `src/vm/Makefile.am`. Though that did not get it included into my main `Makefile` after running `./configure`.
Hmm. To be honest, I didn't look at Makefile.am. I'm getting the source from 'svn://svn.mplayerhq.hu/dvdnav/trunk/libdvdnav'. There is no 'configure' checked in there. There's a 'configure2.sh' but that isn't generating any makefiles. Anyway, I've added the missing file to Makefile.am for completeness. Richard.
On 10/18/13, Richard Hulme <peper03@yahoo.com> wrote:
Hi,
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle."
FWIW there was also this patch: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2012-May/001715.html I wonder if it's related somehow...also what's the status, can it be merged to svn?
Am Montag, den 21.10.2013, 06:51 -0600 schrieb Roger Pack:
On 10/18/13, Richard Hulme <peper03@yahoo.com> wrote:
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle."
FWIW there was also this patch: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2012-May/001715.html I wonder if it's related somehow...
Why should it be related?
also what's the status, can it be merged to svn?
That status of Richard’s or Fabian’s patch? Please check the SVN repository and, if necessary, resubmit Fabian’s patch in a separate thread. Thanks, Paul
On 21/10/13 14:51, Roger Pack wrote:
On 10/18/13, Richard Hulme <peper03@yahoo.com> wrote:
Hi,
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle."
FWIW there was also this patch: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2012-May/001715.html I wonder if it's related somehow...also what's the status, can it be merged to svn?
Hi Roger, No, it's not related (at least not directly). My patch was designed to allow the current state of the virtual machine to be saved and restored to allow DVD bookmarks to be implemented. The patch you reference duplicates the virtual machine and everything else around it. There are several ways the functionality in my patch could be implemented. Saving the binary data would be somewhat more efficient but I wanted to avoid making the dvdnav_s structure public. I also wanted to avoid any issues with data alignment. Returning the data in text form makes it easy to store for almost any system. The data can also be parsed (if necessary) by a human without too much trouble. I felt that a full-on XML implementation was overkill, so just went with a simple format that was fairly straightforward to parse. Richard.
On 10/25/13, Richard Hulme <peper03@yahoo.com> wrote:
On 21/10/13 14:51, Roger Pack wrote:
On 10/18/13, Richard Hulme <peper03@yahoo.com> wrote:
Hi,
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle."
FWIW there was also this patch: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2012-May/001715.html I wonder if it's related somehow...also what's the status, can it be merged to svn?
Hi Roger,
No, it's not related (at least not directly).
My patch was designed to allow the current state of the virtual machine to be saved and restored to allow DVD bookmarks to be implemented. The patch you reference duplicates the virtual machine and everything else around it.
Sounds good. Looks useful. I'll let Fabian chime in if he still wants that other patch applied. (AFAICT it hasn't been)
Roger Pack ha scritto:
On 10/25/13, Richard Hulme <peper03@yahoo.com> wrote:
On 21/10/13 14:51, Roger Pack wrote:
On 10/18/13, Richard Hulme <peper03@yahoo.com> wrote:
Hi,
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle." FWIW there was also this patch: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2012-May/001715.html I wonder if it's related somehow...also what's the status, can it be merged to svn? Hi Roger,
No, it's not related (at least not directly).
My patch was designed to allow the current state of the virtual machine to be saved and restored to allow DVD bookmarks to be implemented. The patch you reference duplicates the virtual machine and everything else around it. Sounds good. Looks useful. I'll let Fabian chime in if he still wants that other patch applied. (AFAICT it hasn't been)
bleah.
Nico Sabbi ha scritto:
Roger Pack ha scritto:
On 10/25/13, Richard Hulme <peper03@yahoo.com> wrote:
On 21/10/13 14:51, Roger Pack wrote:
On 10/18/13, Richard Hulme <peper03@yahoo.com> wrote:
Hi,
The attached patch adds serialization support to libdvdnav, which effectively allows DVD bookmarks to be saved and restored. The current VM state is converted to/from an ASCII string.
This is a patch I created for MythTV, where it is currently in use.
As I wrote in the submit description for MythTV:
"The state snapshot code is borrowed/adapted from or inspired by XBMC and Ogle." FWIW there was also this patch: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2012-May/001715.html
I wonder if it's related somehow...also what's the status, can it be merged to svn? Hi Roger,
No, it's not related (at least not directly).
My patch was designed to allow the current state of the virtual machine to be saved and restored to allow DVD bookmarks to be implemented. The patch you reference duplicates the virtual machine and everything else around it. Sounds good. Looks useful. I'll let Fabian chime in if he still wants that other patch applied. (AFAICT it hasn't been)
bleah.
at least the first patch referenced, in line with handbrake style and build system. I haven't looked at this last one, but the method (exposing a private structure, duplicating and freeing it) is the worst I can imagine. expose apis, not private data. Maybe I had better keep lurking:)
participants (4)
-
Nico Sabbi -
Paul Menzel -
Richard Hulme -
Roger Pack