From: Christophe F. <cfe...@gm...> - 2010-08-30 20:41:57
|
Hi everyone, A libgpod release is long overdue, and I'd like to get one out tomorrow evening (UTC). I still didn't have time to look carefully at Joel Smith's work, and a small part of hawke's bug fixes for album artist/show sorting is also out for now, but I prefer to postpone them post-0.7.94 instead of blocking the release for them (I hope you won't get mad at me for that ;) Anyone using some git version of libgpod, please upgrade to git master and let me know if you encounter any regression with your iPod, and if this happens, please let the list know as soon as possible. Thanks a lot, Christophe |
From: Joel S. <jsf...@jk...> - 2010-08-31 02:24:25
|
On 08/30/2010 02:41 PM, Christophe Fergeau wrote: > A libgpod release is long overdue, and I'd like to get one out tomorrow > evening (UTC). I still didn't have time to look carefully at Joel Smith's > work, and a small part of hawke's bug fixes for album artist/show sorting > is also out for now, but I prefer to postpone them post-0.7.94 instead of > blocking the release for them (I hope you won't get mad at me for that ;) > Of course I'm biased since it's my patches getting left out, but I think that several of my changes are important, especially for TV Shows and Audiobooks on iPhone/iPod Touch. What do you think of committing them on a trial basis post 0.7.94 so that they have a better chance of being tested by a wider audience? Additionally, I'm happy to spend time discussing the patches, and doing alternate versions, if appropriate. The six patches that I have outstanding are: 1. Fix for video_info table to sort by TV Show title Fixes for 959928ee72bb1073982f72574e8bbe2fd3f1389f. If sort_tvshow isn't present, use the TV Show Title for sorting instead. Additionally, create an episode sort value by combining the season number and the episode number. 2. make some WContents functions available to other libgpod modules 3. update chapter parsing to handle (and skip over) child atoms other than name Since chapters can have several child atoms besides name (such as url , urlt, or ploc/trak), the parser now properly skips them and only looks at the name child atom 4. Add chapter support for sqlite tables 5. update plist parsing to return 64-bit ints when a number overflows a 32-bit int 6. Support for reading PlayCounts.plist for play counts, stats and bookmarks. iPhone/iPod Touch 3.x use this file, so without this support, bookmarks are lost during sync. The patches are not all-or-nothing, but they do need to be applied in order in some cases (2 before 4, 5 before 6). #1 and #2 are very straight-forward and shouldn't require much in the way of explanation. #3 isn't a lot of code, but may take some time to read through owing to the nature of the code. #4 is a bunch of new code, but should be harmless even if it is buggy since it's in a different table than is used for anything else. #5 is what I consider to be the riskiest change, since it changes the way a function works, and so if callers were hard-coded to expect a gint gvalue instead of checking the return type, or expecting over/underflow, it could introduce bugs. #6 is all new code, and again, should be harmless in terms of not breaking things that were already working. I and a co-worker have both been using these patches for months without any problems to report. Of course, we only have one class of device to test with (iPhone/iPod Touch) so it'll be nice to have some testing across more devices. Thanks, Joel |
From: Christophe F. <cfe...@gm...> - 2010-08-31 07:23:08
|
Hi Joel, On Mon, Aug 30, 2010 at 08:24:17PM -0600, Joel Smith wrote: > Of course I'm biased since it's my patches getting left out, but I think > that several of my changes are important, especially for TV Shows and > Audiobooks on iPhone/iPod Touch. Yeah, I'm really sorry for that, it takes me a lot of time to get them in while they all seem really useful. One of the reason it took me so long to get a release out is that I didn't want to release without your patches, but for various external reasons (banshee release needing the mono bindings, various distro releases getting closer), it's really better if a release is done now. > What do you think of committing them > on a trial basis post 0.7.94 so that they have a better chance of being > tested by a wider audience? Additionally, I'm happy to spend time > discussing the patches, and doing alternate versions, if appropriate. For most of the patches, I either wanted to spend more time thinking about or to rework the code a bit, that's one of the reason things didn't move really fast with them :-/ > The six patches that I have outstanding are: Yep, I think I've got them all in http://gitorious.org/~teuf/libgpod/teuf-sandbox/commits/iphone > > 1. Fix for video_info table to sort by TV Show title For this one, I'm really not a big fan of comparing for both NULL strings and "\0" strings, and since hawke's patch that didn't make it in was also such a change, I want to spend a bit more time playing with iTunesDBs trying to understand why we get "\0" strings. It seems iTunes writes such strings, but I want to double check that first. > > 2. make some WContents functions available to other libgpod modules Dunno if that's the best course of action for the next patches, if that's done, I think I'd move them to a new file and rename the funcs, I have to play a bit with that and make up my mind > > 3. update chapter parsing to handle (and skip over) child atoms other > than name > > Since chapters can have several child atoms besides name (such as url , > urlt, or ploc/trak), the parser now properly skips them and only looks > at the name child atom On retrospect, I probably should have taken this one ;) > > 4. Add chapter support for sqlite tables I'm not a big fan of putting this adhoc wcontents code in itdb_sqlite, maybe it would be better to have a helper function in itdb_itunesdb.c, maybe not, once again, I need to think more about it and make up my mind. > > 5. update plist parsing to return 64-bit ints when a number overflows a > 32-bit int Yep, I'm uncomfortable with this one, is this needed to handle PlayCounts.plist? Could you send me such a file? > > 6. Support for reading PlayCounts.plist for play counts, stats and > bookmarks. iPhone/iPod Touch 3.x use this file, so without this > support, bookmarks are lost during sync. For this one, I'm not really happy with adding a pcounts2 member to FImport, especially since the way we use FImport::playcounts is awkward so I wanted to use this patch as an opportunity to clean up the existing code. Once again, I'm really sorry for blocking this useful code for so long :-/ I probably should have sent a feedback email much before. I'll try to play with this code this week (maybe the playcount code) ;) Cheers, Christophe |
From: Joel S. <jsf...@jk...> - 2010-08-31 18:40:28
|
Hi Christophe, Sorry for hijacking your release thread. I've renamed the subject similar to the post I originally made regarding the patches. On 08/31/2010 01:23 AM, Christophe Fergeau wrote: > Yeah, I'm really sorry for that, it takes me a lot of time to get them in > while they all seem really useful. One of the reason it took me so long to > get a release out is that I didn't want to release without your patches, > but for various external reasons (banshee release needing the mono > bindings, various distro releases getting closer), it's really better if a > release is done now. > I understand. I agree with "release early, release often". Better to do a release with some features than no release waiting for all features. > For most of the patches, I either wanted to spend more time thinking about > or to rework the code a bit, that's one of the reason things didn't move > really fast with them :-/ > That's fine. If you have specific ideas about how to rework the code, I'm happy to help. Or conversely, if it's something where you want to do it to make sure it's done the way you like, I'll be available for any questions about why I did what I did. As a newcomer to the project, I still don't have a good handle on coding guidelines, practices, etc. so it's no surprise that some of my changes are lacking. >> 1. Fix for video_info table to sort by TV Show title >> > > For this one, I'm really not a big fan of comparing for both NULL strings > and "\0" strings, and since hawke's patch that didn't make it in was also > such a change, I want to spend a bit more time playing with iTunesDBs > trying to understand why we get "\0" strings. It seems iTunes writes such > strings, but I want to double check that first. > I don't remember why I added the check for "\0". It may have been out of ignorance, or I may actually have had such a string. Perhaps I had a database that got into a strange state during development, and a user wouldn't encounter that. I'll investigate and see what I can find. >> 2. make some WContents functions available to other libgpod modules >> > Dunno if that's the best course of action for the next patches, if that's > done, I think I'd move them to a new file and rename the funcs, I have to > play a bit with that and make up my mind > I see your point about renaming, since put_data() (for example) is a pretty generic name to add into a global namespace. >> 4. Add chapter support for sqlite tables >> > I'm not a big fan of putting this adhoc wcontents code in itdb_sqlite, > maybe it would be better to have a helper function in itdb_itunesdb.c, > maybe not, once again, I need to think more about it and make up my mind. > I'm fine either way. If you'd like me to move it to a helper function in itdb_itunesdb.c, that's a pretty straight-forward operation, and I could do a first pass at it. >> 5. update plist parsing to return 64-bit ints when a number overflows a >> 32-bit int >> > Yep, I'm uncomfortable with this one, is this needed to handle > PlayCounts.plist? Could you send me such a file? > It is needed (or something equivalent). The persistentID is stored in the file as an integer value, and the values are 64-bit signed. In my opinion, the plist functions ought to mirror the file format, and if the file format says that an "integer" value can be 64-bit, the function ought to handle that. http://developer.apple.com/iphone/library/documentation/general/conceptual/devpedia-cocoacore/propertylist.html Notice that the same type is used for 32 and 64-bit integers. Fortunately float vs. double is fine since the plist code uses double every time. Perhaps the better change is to have it use gint64 every time instead. This would probably be more invasive as we'd have to find everywhere that an integer value is returned and make sure that the code does the right thing. I'll send you a .plist file off list, but here's an XML snippet of a file (with no guarantee that the dates are reasonable): <key>tracks</key> <array> <dict> <key>audioTrackID</key> <integer>0</integer> <key>bookmarkTimeInMS</key> <integer>0</integer> <key>deleted</key> <false/> <key>persistentID</key> <integer>-8657289912163278308</integer> <key>playCount</key> <integer>1</integer> <key>playMacOSDate</key> <integer>3361651177</integer> <key>playedState</key> <true/> <key>skipCount</key> <integer>0</integer> <key>skipMacOSDate</key> <integer>2082866400</integer> <key>subtitleTrackID</key> <integer>0</integer> <key>userRating</key> <integer>0</integer> </dict> </array> >> 6. Support for reading PlayCounts.plist for play counts, stats and >> bookmarks. iPhone/iPod Touch 3.x use this file, so without this >> support, bookmarks are lost during sync. >> > For this one, I'm not really happy with adding a pcounts2 member to > FImport, especially since the way we use FImport::playcounts is awkward so > I wanted to use this patch as an opportunity to clean up the existing code. > Let me know how I can help. I don't remember why I didn't use the existing member, but it may have been a moment where I wanted to get it working, and didn't properly understand the existing code. If I remember correctly, I couldn't rely upon the ordering of the list like FImport::playcounts does. Also, my aside in #5 about the dates not necessarily being correct reminded me that I need to spend some more time verifying that all the date conversions are correct. > Once again, I'm really sorry for blocking this useful code for so long :-/ > I probably should have sent a feedback email much before. I'll try to play > with this code this week (maybe the playcount code) ;) > No worries... thanks for your help! Regards, Joel |
From: Christophe F. <cfe...@gm...> - 2010-09-04 16:02:48
|
On Sat, Sep 04, 2010 at 10:25:20AM +0200, Christophe Fergeau wrote: > I'll push the chapter code with these latest changes (once I've looked at > it), I haven't pushed the gint64 stuff since it's not really useful for now > without the plist playcount code but it's good to go, the "skip name atom" is > good too. As for the playcount code, it looked good too, but I want to go > once more over it. Ok, I've reworked the chapterdata patch and the playcount patch to my liking (mainly by slight code movements), let me know if it's fine with you, all that remains to be done is to squash the "simplify playcounts_plist_read" commit with the previous one, and everything up to that commit is good to go for me. These commits are all in my sandbox:iphone branch. Christophe |
From: Christophe F. <cfe...@gm...> - 2010-09-18 16:08:09
|
On Sat, Sep 04, 2010 at 06:02:40PM +0200, Christophe Fergeau wrote: > Ok, I've reworked the chapterdata patch and the playcount patch to my > liking (mainly by slight code movements), let me know if it's fine with > you, all that remains to be done is to squash the "simplify > playcounts_plist_read" commit with the previous one, and everything up to > that commit is good to go for me. These commits are all in my > sandbox:iphone branch. I pushed these patches after doing the aforementioned squash, let me know if you had objections related to that :) Christophe |
From: Christophe F. <cfe...@gm...> - 2010-09-04 08:25:27
|
On Fri, Sep 03, 2010 at 07:26:12PM -0600, Joel Smith wrote: > On 09/03/2010 03:19 PM, Christophe Fergeau wrote: > >Ah, you shouldn't have mentioned that, now I'm going to say that this > >duplicated code should be put in a function used by both codepaths instead > >of being copied and pasted :) This looks easy enough to do, just let me > >know if you don't have time for that. > > My first pass cut a little too deeply, and I forgot to put the > "reversed" back in. Hopefully my next try is a little closer. My > master sandbox branch has the separate commits, but once we are > happy with the code, I think that the commits ought to look more > like this: > http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox/commits/iphone > One to move the chapter code to a helper function, and another to > add sqlite chapter support using the helper function. Yep, this is exactly what I wanted to do. > How close are we to pushing any or all of these to master? I'll push the chapter code with these latest changes (once I've looked at it), I haven't pushed the gint64 stuff since it's not really useful for now without the plist playcount code but it's good to go, the "skip name atom" is good too. As for the playcount code, it looked good too, but I want to go once more over it. And I haven't looked at the video ordering patch yet. So most of the patches are mostly ready to go in:) Christophe |
From: Christophe F. <cfe...@gm...> - 2010-08-31 19:29:40
|
On Tue, Aug 31, 2010 at 12:40:17PM -0600, Joel Smith wrote: > >> 4. Add chapter support for sqlite tables > >> > > I'm not a big fan of putting this adhoc wcontents code in itdb_sqlite, > > maybe it would be better to have a helper function in itdb_itunesdb.c, > > maybe not, once again, I need to think more about it and make up my mind. > > > I'm fine either way. If you'd like me to move it to a helper function > in itdb_itunesdb.c, that's a pretty straight-forward operation, and I > could do a first pass at it. If you are bored and have a bit of free time to try this, I'd like to see how the patch looks like to compare the 2 approaches. This means one of them will be dropped, and maybe that won't be the last one you wrote. If you have better things to do, that's fine, I can do it, that's a good opportunity to look closer at the code anyway. > Perhaps the better change is to have it use gint64 every > time instead. This would probably be more invasive as we'd have to find > everywhere that an integer value is returned and make sure that the code > does the right thing. Yep, I'd go this way, I'm hopeful the switch won't be too hard and that there won't be breakage. ./tests/test-sysinfo-extended-parsing on a SysInfoExtended file from an iphone should run most of the relevant code. This is http://gitorious.org/~teuf/libgpod/teuf-sandbox/commit/b60b7fa4b3efe8300326a6e5cd35d01b2e988e3d (untested code ;) > > >> 6. Support for reading PlayCounts.plist for play counts, stats and > >> bookmarks. iPhone/iPod Touch 3.x use this file, so without this > >> support, bookmarks are lost during sync. > >> > > For this one, I'm not really happy with adding a pcounts2 member to > > FImport, especially since the way we use FImport::playcounts is awkward so > > I wanted to use this patch as an opportunity to clean up the existing code. > > > > Let me know how I can help. I don't remember why I didn't use the > existing member, but it may have been a moment where I wanted to get it > working, and didn't properly understand the existing code. If I > remember correctly, I couldn't rely upon the ordering of the list like > FImport::playcounts does. Everytime I look at the playcount code, I get confused, so it's not too surprising that you gave up on the old code and just did your own stuff. > Also, my aside in #5 about the dates not > necessarily being correct reminded me that I need to spend some more > time verifying that all the date conversions are correct. There are conversion functions in itdb_sqlite.c and in itdb_device.c, hopefully one of the 2 will do. Christophe |
From: Christophe F. <cfe...@gm...> - 2010-09-01 19:53:28
|
2010/8/31 Christophe Fergeau <cfe...@gm...>: > On Tue, Aug 31, 2010 at 12:40:17PM -0600, Joel Smith wrote: >> >> 6. Support for reading PlayCounts.plist for play counts, stats and >> >> bookmarks. iPhone/iPod Touch 3.x use this file, so without this >> >> support, bookmarks are lost during sync. >> >> >> > For this one, I'm not really happy with adding a pcounts2 member to >> > FImport, especially since the way we use FImport::playcounts is awkward so >> > I wanted to use this patch as an opportunity to clean up the existing code. >> > >> >> Let me know how I can help. I don't remember why I didn't use the >> existing member, but it may have been a moment where I wanted to get it >> working, and didn't properly understand the existing code. If I >> remember correctly, I couldn't rely upon the ordering of the list like >> FImport::playcounts does. > > Everytime I look at the playcount code, I get confused, so it's not too > surprising that you gave up on the old code and just did your own stuff. Ok, I reread the code and it seems indeed difficult to unify the 2 approaches, I guess we'll have to live with the 2 members. Though it's probably a good time to add an itdb_playcount.c file to "hide" all this code. Christophe |
From: Joel S. <jsf...@jk...> - 2010-09-02 18:45:40
|
On 08/31/2010 01:29 PM, Christophe Fergeau wrote: > On Tue, Aug 31, 2010 at 12:40:17PM -0600, Joel Smith wrote: > >>>> 4. Add chapter support for sqlite tables >>>> >>>> >>> I'm not a big fan of putting this adhoc wcontents code in itdb_sqlite, >>> maybe it would be better to have a helper function in itdb_itunesdb.c, >>> maybe not, once again, I need to think more about it and make up my mind. >>> >>> >> I'm fine either way. If you'd like me to move it to a helper function >> in itdb_itunesdb.c, that's a pretty straight-forward operation, and I >> could do a first pass at it. >> > If you are bored and have a bit of free time to try this, I'd like to see > how the patch looks like to compare the 2 approaches. This means one of > them will be dropped, and maybe that won't be the last one you wrote. If > you have better things to do, that's fine, I can do it, that's a good > opportunity to look closer at the code anyway. > I have just pushed some new commits up to my sandbox at http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox I removed the old commits for chapter support and wcontents and refactored the code to use a helper function instead. IMHO, this approach is cleaner and more straight-forward than what I had before. >> Perhaps the better change is to have it use gint64 every >> time instead. This would probably be more invasive as we'd have to find >> everywhere that an integer value is returned and make sure that the code >> does the right thing. >> > Yep, I'd go this way, I'm hopeful the switch won't be too hard and that > there won't be breakage. ./tests/test-sysinfo-extended-parsing on a > SysInfoExtended file from an iphone should run most of the relevant > code. This is > http://gitorious.org/~teuf/libgpod/teuf-sandbox/commit/b60b7fa4b3efe8300326a6e5cd35d01b2e988e3d > (untested code ;) > I tested this code and found a bug which caused all integer values to become zero since the types didn't match in get_int. I have a proposed fix in my sandbox. It's here: http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox/commit/34d0fe3f66a9f361c330c1cf7279a3a40b1f30e8 Maybe we should squash all three gint64 plist commits into one before applying it to master. Joel |
From: Christophe F. <cfe...@gm...> - 2010-09-02 19:08:55
|
On Thu, Sep 02, 2010 at 12:45:30PM -0600, Joel Smith wrote: > I have just pushed some new commits up to my sandbox at > http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox > I removed the old commits for chapter support and wcontents and > refactored the code to use a helper function instead. IMHO, this > approach is cleaner and more straight-forward than what I had before. Ah good to know, I'll look at it then :) > > I tested this code and found a bug which caused all integer values to > become zero since the types didn't match in get_int. I have a proposed > fix in my sandbox. It's here: > http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox/commit/34d0fe3f66a9f361c330c1cf7279a3a40b1f30e8 > Maybe we should squash all three gint64 plist commits into one before > applying it to master. Thanks a lot for debugging this, I always feel bad when I let others do the dirty work :-/ Yep, I'll squash them, I didn't do it initially so that you can see more easily what was changed compared to your patch. Christophe |
From: Christophe F. <cfe...@gm...> - 2010-09-02 19:50:04
|
On Thu, Sep 02, 2010 at 12:45:30PM -0600, Joel Smith wrote: > I have just pushed some new commits up to my sandbox at > http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox > I removed the old commits for chapter support and wcontents and > refactored the code to use a helper function instead. IMHO, this > approach is cleaner and more straight-forward than what I had before. I looked at it, and I indeed prefer it this way, a few very minor comments below: index 4f6d1e8..3a79962 100644 --- a/src/itdb.h +++ b/src/itdb.h @@ -1990,6 +1990,11 @@ gboolean itdb_chapterdata_add_chapter (Itdb_Chapterdata *chapterdata, guint32 startpos, gchar *chaptertitle); +/* Create an itdb chapterdata blob suitable for storage in SQlite + * tables. */ +GString *itdb_build_chapter_blob(Itdb_Chapterdata *chapterdata, + gboolean reversed); it's not worth exporting it, I'd just make it G_GNUC_INTERNAL and put it in a header which is not installed (itdb_private.h maybe). itdb_chapterdata_build_chapter_blob would be better. The "reversed" argument can be dropped if I'm not mistaken, the only reason there is all this "reversed" stuff is to handle the old Motorola ROKR phones, for iOS, I don't think endianness will change soon (I hope I'm not mistaken when I assume "reversed" is about iPod endianness, not host endianness :) + #ifndef LIBGPOD_DISABLE_DEPRECATED /* time functions */ time_t itdb_time_get_mac_time (void); diff --git a/src/itdb_itunesdb.c b/src/itdb_itunesdb.c index bcb45c5..7e32293 100644 --- a/src/itdb_itunesdb.c +++ b/src/itdb_itunesdb.c + put32lint (cts, chapterdata->unk024); /* unknown */ + put32lint (cts, chapterdata->unk028); /* unknown */ + put32lint (cts, chapterdata->unk032); /* unknown */ + header_seek = cts->pos; /* needed to fix length */ + put32bint (cts, -1); /* total length of sean atom, fix later */ + put_header (cts, "sean"); + put32bint (cts, 1); /* unknown */ + put32bint (cts, numchapters+1); /* children */ + put32bint (cts, 0); /* unknown */ There really is a mix of little endian and big endian values? This is nasty :-/ + for (ch_gl=chapterdata->chapters; ch_gl; ch_gl=ch_gl->next) + { + gunichar2 *title_utf16; + Itdb_Chapter *chapter = ch_gl->data; + glong len; + title_utf16 = NULL; Unneeded + title_utf16 = g_utf8_to_utf16 (chapter->chaptertitle, -1, NULL, &len, NULL); + fixup_big_utf16 (title_utf16); + put32bint (cts, 42+2*len); /* total length */ + put_header (cts, "chap"); + put32bint (cts, chapter->startpos); /* should we check if startpos=0 here? */ + put32bint (cts, 1); /* children */ + put32bint (cts, 0); /* unknown */ + put32bint (cts, 22+2*len); /* length */ + put_header (cts, "name"); + put32bint (cts, 1); /* unknown */ + put32_n0 (cts, 2); /* unknown */ + put16bint (cts, len); + put_data (cts, (gchar *)title_utf16, 2*len); + g_free (title_utf16); + } + put32bint (cts, 28); /* size */ + put_header (cts, "hedr"); + put32bint (cts, 1); /* unknown */ + put32bint (cts, 0); /* children */ + put32_n0 (cts, 2); /* unknown */ + put32bint (cts, 1); /* unknown */ + + put32bint_seek (cts, cts->pos-header_seek, header_seek); /* size */ + + idx = 0; I don't think this is needed either. Thanks for all the work so far :) Christophe |
From: Bastien N. <ha...@ha...> - 2010-09-02 20:26:00
|
On Thu, 2010-09-02 at 21:49 +0200, Christophe Fergeau wrote: > On Thu, Sep 02, 2010 at 12:45:30PM -0600, Joel Smith wrote: > > I have just pushed some new commits up to my sandbox at > > http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox > > I removed the old commits for chapter support and wcontents and > > refactored the code to use a helper function instead. IMHO, this > > approach is cleaner and more straight-forward than what I had before. > > I looked at it, and I indeed prefer it this way, a few very minor comments > below: > > index 4f6d1e8..3a79962 100644 > --- a/src/itdb.h > +++ b/src/itdb.h > @@ -1990,6 +1990,11 @@ gboolean itdb_chapterdata_add_chapter (Itdb_Chapterdata *chapterdata, > guint32 startpos, > gchar *chaptertitle); > > +/* Create an itdb chapterdata blob suitable for storage in SQlite > + * tables. */ > +GString *itdb_build_chapter_blob(Itdb_Chapterdata *chapterdata, > + gboolean reversed); What's the point of giving back a GString when you're not passing in a GString? If you were hoping to do incremental building, then GString in, GString out is a good idea. Otherwise you're better off destroying the GString and getting the data out of it. |
From: Christophe F. <cfe...@gm...> - 2010-09-02 20:51:06
|
2010/9/2 Bastien Nocera <ha...@ha...>: > What's the point of giving back a GString when you're not passing in a > GString? If you were hoping to do incremental building, then GString in, > GString out is a good idea. Otherwise you're better off destroying the > GString and getting the data out of it. That's only used as a convenient way of returning a char * + length (the char * containing binary data), it's destroyed right after that. It's not the perfect type to carry this kind of data, but this works well enough, I guess the alternative would be to have guchar **data, gsize *len arguments ? Christophe |
From: Joel S. <jsf...@jk...> - 2010-09-02 21:09:57
|
On 09/02/2010 02:50 PM, Christophe Fergeau wrote: > 2010/9/2 Bastien Nocera<ha...@ha...>: > >> What's the point of giving back a GString when you're not passing in a >> GString? If you were hoping to do incremental building, then GString in, >> GString out is a good idea. Otherwise you're better off destroying the >> GString and getting the data out of it. >> > That's only used as a convenient way of returning a char * + length > (the char * containing binary data), it's destroyed right after that. > It's not the perfect type to carry this kind of data, but this works > well enough, I guess the alternative would be to have guchar **data, > gsize *len arguments ? > I'm not an experienced GTK programmer, so if there's a well-established or better way to do it, it's fine by me, but Christophe is right... I just wanted to return a buffer + length. I figured it would be efficient since the GString is created with a known length up front, so it should be nearly equivalent to allocating a buffer of the right size and returning that. But it is two allocs/frees instead of one, so maybe that is an argument to change it. Joel |
From: Joel S. <jsf...@jk...> - 2010-09-02 21:02:15
|
On 09/02/2010 01:49 PM, Christophe Fergeau wrote: > +GString *itdb_build_chapter_blob(Itdb_Chapterdata *chapterdata, > + gboolean reversed); > > it's not worth exporting it, I'd just make it G_GNUC_INTERNAL and put it in > a header which is not installed (itdb_private.h maybe). > itdb_chapterdata_build_chapter_blob would be better. > That's fine. I've changed it to match your suggestions. > The "reversed" argument can be dropped if I'm not mistaken, the only reason > there is all this "reversed" stuff is to handle the old Motorola ROKR > phones, for iOS, I don't think endianness will change soon (I hope I'm not > mistaken when I assume "reversed" is about iPod endianness, not host > endianness :) > > I got the bulk of the code from itdb chapter support code, so in many cases I did things only because I was copying what was done in the itdb chapter code. I agree that this is referring to iPod endianness, and I expect you're right about it not changing down the road. It'll be easy enough to add back in if needed. > + put32lint (cts, chapterdata->unk028); /* unknown */ > + put32lint (cts, chapterdata->unk032); /* unknown */ > + header_seek = cts->pos; /* needed to fix length */ > + put32bint (cts, -1); /* total length of sean atom, fix later */ > + put_header (cts, "sean"); > + put32bint (cts, 1); /* unknown */ > + put32bint (cts, numchapters+1); /* children */ > + put32bint (cts, 0); /* unknown */ > > There really is a mix of little endian and big endian values? This is nasty > :-/ > Again, this was code that was copied from the other chapter code. It seems that only the 3 unknown values are little endian, and I'm not familiar with the history of the code to be able to say why that is. > + for (ch_gl=chapterdata->chapters; ch_gl; ch_gl=ch_gl->next) > + { > + gunichar2 *title_utf16; > + Itdb_Chapter *chapter = ch_gl->data; > + glong len; > + title_utf16 = NULL; > > Unneeded > I noticed the same thing but left it there in the interest of making my changes be minimal relative to what I'd copied from. I'll remove it from there and the original source. > + idx = 0; > > I don't think this is needed either. > Good catch. I thought I had cleaned that up. I don't know how that one managed to sneak past me. idx isn't even needed here at all since it's for the sqlite code. Also, I somehow managed to revert http://gitorious.org/~teuf/libgpod/teuf-sandbox/commit/194be52ed1649e1a88fc91598e289a6b268f9aaf in my PlayCounts.plist commit. Sorry about that... I'm still not very good at using git with pulling from one place and pushing to another. My current sandbox has an updated commit without that bad change. Thanks, Joel |
From: Bastien N. <ha...@ha...> - 2010-09-02 21:07:25
|
On Thu, 2010-09-02 at 22:50 +0200, Christophe Fergeau wrote: > 2010/9/2 Bastien Nocera <ha...@ha...>: > > What's the point of giving back a GString when you're not passing in a > > GString? If you were hoping to do incremental building, then GString in, > > GString out is a good idea. Otherwise you're better off destroying the > > GString and getting the data out of it. > > That's only used as a convenient way of returning a char * + length > (the char * containing binary data), it's destroyed right after that. > It's not the perfect type to carry this kind of data, but this works > well enough, I guess the alternative would be to have guchar **data, > gsize *len arguments ? You could probably use GByteArray instead then. Or char * as a return, and gsize *len as an out arg. |
From: Joel S. <jsf...@jk...> - 2010-09-02 22:20:15
|
On 09/02/2010 03:03 PM, Bastien Nocera wrote: > You could probably use GByteArray instead then. Or char * as a return, > and gsize *len as an out arg. > I switched my version to use GByteArray instead. I see the GString version as being substantially equivalent, but perhaps the intent is more clearly conveyed by using GByteArray. Thanks for the suggestion. Joel |
From: Christophe F. <cfe...@gm...> - 2010-09-03 21:19:42
|
On Thu, Sep 02, 2010 at 03:02:04PM -0600, Joel Smith wrote: > I got the bulk of the code from itdb chapter support code, so in many > cases I did things only because I was copying what was done in the itdb > chapter code. Ah, you shouldn't have mentioned that, now I'm going to say that this duplicated code should be put in a function used by both codepaths instead of being copied and pasted :) This looks easy enough to do, just let me know if you don't have time for that. Thanks! Christophe |
From: Joel S. <jsf...@jk...> - 2010-09-04 01:26:22
|
On 09/03/2010 03:19 PM, Christophe Fergeau wrote: > Ah, you shouldn't have mentioned that, now I'm going to say that this > duplicated code should be put in a function used by both codepaths instead > of being copied and pasted :) This looks easy enough to do, just let me > know if you don't have time for that. > My first pass cut a little too deeply, and I forgot to put the "reversed" back in. Hopefully my next try is a little closer. My master sandbox branch has the separate commits, but once we are happy with the code, I think that the commits ought to look more like this: http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox/commits/iphone One to move the chapter code to a helper function, and another to add sqlite chapter support using the helper function. I don't have a device to test with that uses the regular iTunes DB instead of sqlite, so I don't have a good way of verifying the changes. How close are we to pushing any or all of these to master? Cheers, Joel |
From: Joel S. <jsf...@jk...> - 2010-09-03 21:40:05
|
On 09/03/2010 03:19 PM, Christophe Fergeau wrote: > Ah, you shouldn't have mentioned that, now I'm going to say that this > duplicated code should be put in a function used by both codepaths instead > of being copied and pasted :) This looks easy enough to do, just let me > know if you don't have time for that. > My only hesitation is that I didn't want to worry about a future change in the traditional itdb codepath breaking the sqlite codepath, since they aren't necessarily identical. But I suppose that until (if ever) the formats diverge, it's safe enough to have a common code path. Additionally, I'll need to re-add the reversed param since the other codepath needs it. If you're convinced that this is the right direction to take the code, I'm fine doing it. Joel |
From: Joel S. <jsf...@jk...> - 2010-09-03 22:02:12
|
On 09/03/2010 03:39 PM, Joel Smith wrote: > On 09/03/2010 03:19 PM, Christophe Fergeau wrote: > >> Ah, you shouldn't have mentioned that, now I'm going to say that this >> duplicated code should be put in a function used by both codepaths instead >> of being copied and pasted :) This looks easy enough to do, just let me >> know if you don't have time for that. >> >> > My only hesitation is that I didn't want to worry about a future change > in the traditional itdb codepath breaking the sqlite codepath, since > they aren't necessarily identical. But I suppose that until (if ever) > the formats diverge, it's safe enough to have a common code path. > Additionally, I'll need to re-add the reversed param since the other > codepath needs it. If you're convinced that this is the right direction > to take the code, I'm fine doing it. Have a look, and see what you think. http://gitorious.org/~joelsmith/libgpod/joelsmith-sandbox/commit/6afd6731307bcf632f60fb63bcf4f258ccb8e8c5 Joel |