gtkpod-devel — gtkpod development issues

Re: [Gtkpod-devel] [PATCH 0/3] Fixing a few crash bugs

[phantomjinx <p.g...@ph...>]

"Jonathan Neuschäfer" <j.n...@gm...> wrote:

>Phantomjinx,
>
>please consider including this patchset in 2.1.4:
>
>On Thu, Dec 20, 2012 at 01:08:25PM +0100, Jonathan Neuschäfer wrote:
>> With this patchset I fix three cases in which Gtkpod could crash.
>> It is also available on github:
>> 
>> 	https://github.com/neuschaefer/gtkpod/commits/dev
>> 
>> Patches:
>> * de9884f track_display: handle gtkpod_get_current_playlist() == NULL
>> * 6323646 coverart: fix a crash on missing artist names
>> * 09b7b9d playlist_display: prevent a double-free
>
>
>Thanks,
>Jonathan Neuschäfer
>
>------------------------------------------------------------------------------
>Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
>MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
>with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
>MVPs and experts. ON SALE this month only -- learn more at:
>http://p.sf.net/sfu/learnmore_123012
>_______________________________________________
>Gtkpod-devel mailing list
>Gtk...@li...
>https://lists.sourceforge.net/lists/listinfo/gtkpod-devel

Hi Jonathan,

 I did send you a message regarding these patches last night with an apology for not including them in 2.1.3. Did you not get it?

Did you want gtkpod commit access?

Thanks for your efforts.

PGR

Re: [Gtkpod-devel] [PATCH 0/3] Fixing a few crash bugs

[Jonathan N. <j.n...@gm...>]

Phantomjinx,

please consider including this patchset in 2.1.4:

On Thu, Dec 20, 2012 at 01:08:25PM +0100, Jonathan Neuschäfer wrote:
> With this patchset I fix three cases in which Gtkpod could crash.
> It is also available on github:
> 
> 	https://github.com/neuschaefer/gtkpod/commits/dev
> 
> Patches:
> * de9884f track_display: handle gtkpod_get_current_playlist() == NULL
> * 6323646 coverart: fix a crash on missing artist names
> * 09b7b9d playlist_display: prevent a double-free


Thanks,
Jonathan Neuschäfer

[Gtkpod-devel] gtkpod-2.1.3

[phantomjinx <p.g...@ph...>]

Hi all,

I have pushed out the final version of gtkpod 2.1.3. It does differ from the beta 1 in that I added
a fix to a bug (reported by Götz - crash when deleting all tracks).

Have also updated the version script to 2.1.4 so any new work, please go ahead and commit.

Cheers

PGR

-- 
Paul Richardson

  * p.g...@ph...
  * p.g...@re...
  * pgr...@li...

"I know exactly who reads the papers ...

  * The Daily Mirror is read by people who think they run the country.
  * The Guardian is read by people who think they ought to run the country.
  * The Times is read by people who do actually run the country.
  * The Daily Mail is read by the wives of the people who run the country.
  * The Financial Times is read by the people who own the country.
  * The Morning Star is read by the people who think the country ought to be run by another country.
  * The Daily Telegraph is read by the people who think it is."

Jim Hacker, Yes Minister

Re: [Gtkpod-devel] different versions of AtomicParsley

[phantomjinx <p.g...@ph...>]

On 01/02/2013 02:07 AM, Jonathan Neuschäfer wrote:
> Hello PGR (and other devs),
> 
> the version of AtomicParsley that you included in Gtkpod 2.1.2 seems to
> be unmaintained since 2006. AFAICT Wez Furlong has pracitically over-
> taken development:
> 
> 	https://bitbucket.org/wez/atomicparsley/overview
> 
> (Releases are available under Downloads -> Tags.)
> 
> Please tell me your opinion on upgrading gtkpod's copy of AP to that
> version.
> 
> 
> Thanks,
> Jonathan Neuschäfer
> 
> ------------------------------------------------------------------------------
> Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery
> and much more. Keep your Java skills current with LearnJavaNow -
> 200+ hours of step-by-step video tutorials by Java experts.
> SALE $49.99 this month only -- learn more at:
> http://p.sf.net/sfu/learnmore_122612 
> _______________________________________________
> Gtkpod-devel mailing list
> Gtk...@li...
> https://lists.sourceforge.net/lists/listinfo/gtkpod-devel
> 

Hi Jonathan,

Certainly worth looking into I think. Other than the bridge (AtomicParsleyBridge.cpp) and bugfixes
to get AtomicParsley working, I did very little with it.

Noticed that the layout of the code is different so there will be a bit of work matching up the
files and their functions. If you would like to look into it then it would be much appreciated.
Otherwise, I'll pop it on the TODO list.

Should hopefully get 2.1.2 out soon.

Cheers

Paul

-- 
Paul Richardson

  * p.g...@ph...
  * p.g...@re...
  * pgr...@li...

"I know exactly who reads the papers ...

  * The Daily Mirror is read by people who think they run the country.
  * The Guardian is read by people who think they ought to run the country.
  * The Times is read by people who do actually run the country.
  * The Daily Mail is read by the wives of the people who run the country.
  * The Financial Times is read by the people who own the country.
  * The Morning Star is read by the people who think the country ought to be run by another country.
  * The Daily Telegraph is read by the people who think it is."

Jim Hacker, Yes Minister

[Gtkpod-devel] different versions of AtomicParsley

[Jonathan N. <j.n...@gm...>]

Hello PGR (and other devs),

the version of AtomicParsley that you included in Gtkpod 2.1.2 seems to
be unmaintained since 2006. AFAICT Wez Furlong has pracitically over-
taken development:

	https://bitbucket.org/wez/atomicparsley/overview

(Releases are available under Downloads -> Tags.)

Please tell me your opinion on upgrading gtkpod's copy of AP to that
version.


Thanks,
Jonathan Neuschäfer

[Gtkpod-devel] [PATCH 3/3] playlist_display: prevent a double-free

[Jonathan N. <j.n...@gm...>]

This crash could previously be triggered by dragging a newly
created (and not yet saved) playlist from one local repository
to another.
---
 plugins/playlist_display/display_playlists.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/playlist_display/display_playlists.c b/plugins/playlist_display/display_playlists.c
index 380834e..f2d1300 100644
--- a/plugins/playlist_display/display_playlists.c
+++ b/plugins/playlist_display/display_playlists.c
@@ -765,6 +765,7 @@ static void pm_drag_data_received(GtkWidget *widget, GdkDragContext *dc, gint x,
             if (pos == GTK_TREE_VIEW_DROP_AFTER)
                 pl_d = gp_playlist_add_new(pl->itdb, pl_s->name, FALSE, position + 1);
             g_free(data_copy);
+            data_copy = NULL;
             g_return_if_fail (pl_d);
 
             /* copy files from iPod if necessary */
-- 
1.7.10.4

[Gtkpod-devel] [PATCH 0/3] Fixing a few crash bugs

[Jonathan N. <j.n...@gm...>]

With this patchset I fix three cases in which Gtkpod could crash.
It is also available on github:

	https://github.com/neuschaefer/gtkpod/commits/dev

Patches:
* de9884f track_display: handle gtkpod_get_current_playlist() == NULL
* 6323646 coverart: fix a crash on missing artist names
* 09b7b9d playlist_display: prevent a double-free


Thanks,
Jonathan Neuschäfer

[Gtkpod-devel] [PATCH 2/3] coverart: fix a crash on missing artist names

[Jonathan N. <j.n...@gm...>]

---
 plugins/cover_display/display_coverart.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/plugins/cover_display/display_coverart.c b/plugins/cover_display/display_coverart.c
index f981817..e3b8dcb 100644
--- a/plugins/cover_display/display_coverart.c
+++ b/plugins/cover_display/display_coverart.c
@@ -533,9 +533,11 @@ void coverart_display_update(gboolean clear_track_list) {
 
         while (tracks) {
             gchar *album_key;
+            gchar *artist_name;
             track = tracks->data;
 
-            album_key = g_strconcat(track->artist, "_", track->album, NULL);
+            artist_name = track->artist? track->artist : "";
+            album_key = g_strconcat(artist_name, "_", track->album, NULL);
             /* Check whether an album item has already been created in connection
              * with the track's artist and album
              */
-- 
1.7.10.4

[Gtkpod-devel] [PATCH 1/3] track_display: handle gtkpod_get_current_playlist() == NULL

[Jonathan N. <j.n...@gm...>]

---
 plugins/track_display/display_tracks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/track_display/display_tracks.c b/plugins/track_display/display_tracks.c
index 122e3a8..be57bff 100644
--- a/plugins/track_display/display_tracks.c
+++ b/plugins/track_display/display_tracks.c
@@ -1157,7 +1157,7 @@ static void tm_adopt_order(GList *tracks) {
      * list.
      */
     Playlist *cp = gtkpod_get_current_playlist();
-    GList *pt = cp->members;
+    GList *pt = cp? cp->members : NULL;
     GtkTreeModel *model = gtk_tree_view_get_model(track_treeview);
     while (pt != NULL) {
         Track *track = pt->data;
-- 
1.7.10.4

Re: [Gtkpod-devel] NEWS and (vs.?) ChangeLog

[Jonathan N. <j.n...@gm...>]

On Tue, Dec 18, 2012 at 07:20:35AM +0000, phantomjinx wrote:
> "Jonathan Neuschäfer" <j.n...@gm...> wrote:
> 
> >Hi everyone,
> >
> >Fixing a few crash bugs (patches are yet to come), I noticed Gtkpod has
> >a "NEWS" file and also a "ChangeLog" file.
> >What is the specific purpose of each of the files?
> >Whould it make sense to keep the NEWS file and replace the ChangeLog
> >file by a script that extracts a change log out of the Git history?
> >
> >
> >Thanks in advance,
> >Jonathan Neuschäfer
> >
> 
> Hi Jonathan,
> 
> The NEWS file is a summary of changes for each version while the change log is the git history. There I'd already a script for generating the log, which I run to update the changelog then commit it before a release. 

Okay, that's fine. Something made me think the ChangeLog was updated
manually, so thanks again for the info.

Jonathan Neuschäfer

Re: [Gtkpod-devel] NEWS and (vs.?) ChangeLog

[phantomjinx <p.g...@ph...>]

"Jonathan Neuschäfer" <j.n...@gm...> wrote:

>Hi everyone,
>
>Fixing a few crash bugs (patches are yet to come), I noticed Gtkpod has
>a "NEWS" file and also a "ChangeLog" file.
>What is the specific purpose of each of the files?
>Whould it make sense to keep the NEWS file and replace the ChangeLog
>file by a script that extracts a change log out of the Git history?
>
>
>Thanks in advance,
>Jonathan Neuschäfer
>
>------------------------------------------------------------------------------
>LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
>Remotely access PCs and mobile devices and provide instant support
>Improve your efficiency, and focus on delivering more value-add
>services
>Discover what IT Professionals Know. Rescue delivers
>http://p.sf.net/sfu/logmein_12329d2d
>_______________________________________________
>Gtkpod-devel mailing list
>Gtk...@li...
>https://lists.sourceforge.net/lists/listinfo/gtkpod-devel

Hi Jonathan,

The NEWS file is a summary of changes for each version while the change log is the git history. There I'd already a script for generating the log, which I run to update the changelog then commit it before a release. 

Both have been standard files in a project of this kind so that's why they exist.

Hope that helps.

Thanks

PGR

[Gtkpod-devel] NEWS and (vs.?) ChangeLog

[Jonathan N. <j.n...@gm...>]

Hi everyone,

Fixing a few crash bugs (patches are yet to come), I noticed Gtkpod has
a "NEWS" file and also a "ChangeLog" file.
What is the specific purpose of each of the files?
Whould it make sense to keep the NEWS file and replace the ChangeLog
file by a script that extracts a change log out of the Git history?


Thanks in advance,
Jonathan Neuschäfer

Re: [Gtkpod-devel] gtkpod 2.1.3-beta1

[phantomjinx <p.g...@ph...>]

On 12/07/2012 08:12 AM, Götz Waschk wrote:
> On Mon, Dec 3, 2012 at 12:11 AM, phantomjinx > Finally, have got
> around to releasing a beta of 2.1.3 to sourceforge. This is mainly a
> maintenance
>> release with no new features but some recent and long term bugs have now been fixed, including
>> sorting in the track display view which has long been a source of confusion!
>>
>> So, if anyone can please download and test and post back any feedback they may have. Might manage
>> the release around the holidays at the end of the month subject to feedback.
> 
> Hi Paul,
> 
> I have tested on Mageia 2 (I'm the new maintainer for this
> distribution) and it crashed when I was trying to delete all tracks
> from the Podcasts playlist. Please see the attached backtrace. I'm
> using gtk+ 3.4.1.
> 
> Regards, Götz
> 

Thanks. I'll look at replicating and fixing it before the release.

Regards

PGR


-- 
Paul Richardson

  * p.g...@ph...
  * p.g...@re...
  * pgr...@li...

"I know exactly who reads the papers ...

  * The Daily Mirror is read by people who think they run the country.
  * The Guardian is read by people who think they ought to run the country.
  * The Times is read by people who do actually run the country.
  * The Daily Mail is read by the wives of the people who run the country.
  * The Financial Times is read by the people who own the country.
  * The Morning Star is read by the people who think the country ought to be run by another country.
  * The Daily Telegraph is read by the people who think it is."

Jim Hacker, Yes Minister

Re: [Gtkpod-devel] gtkpod 2.1.3-beta1

[Götz W. <goe...@gm...>]

On Mon, Dec 3, 2012 at 12:11 AM, phantomjinx > Finally, have got
around to releasing a beta of 2.1.3 to sourceforge. This is mainly a
maintenance
> release with no new features but some recent and long term bugs have now been fixed, including
> sorting in the track display view which has long been a source of confusion!
>
> So, if anyone can please download and test and post back any feedback they may have. Might manage
> the release around the holidays at the end of the month subject to feedback.

Hi Paul,

I have tested on Mageia 2 (I'm the new maintainer for this
distribution) and it crashed when I was trying to delete all tracks
from the Podcasts playlist. Please see the attached backtrace. I'm
using gtk+ 3.4.1.

Regards, Götz
-- 
AL I:40: Do what thou wilt shall be the whole of the Law.

[Gtkpod-devel] gtkpod 2.1.3-beta1

[phantomjinx <p.g...@ph...>]

Evening all,

Finally, have got around to releasing a beta of 2.1.3 to sourceforge. This is mainly a maintenance
release with no new features but some recent and long term bugs have now been fixed, including
sorting in the track display view which has long been a source of confusion!

So, if anyone can please download and test and post back any feedback they may have. Might manage
the release around the holidays at the end of the month subject to feedback.

Download -> http://sourceforge.net/projects/gtkpod/files/gtkpod/gtkpod-2.1.3-beta1.tar.gz

Thanks to all those who have contributed to this release. You are all mentioned in the git commits
and where possible the NEWS log.

Cheers

PGR

-- 
Paul Richardson

  * p.g...@ph...
  * p.g...@re...
  * pgr...@li...

"I know exactly who reads the papers ...

  * The Daily Mirror is read by people who think they run the country.
  * The Guardian is read by people who think they ought to run the country.
  * The Times is read by people who do actually run the country.
  * The Daily Mail is read by the wives of the people who run the country.
  * The Financial Times is read by the people who own the country.
  * The Morning Star is read by the people who think the country ought to be run by another country.
  * The Daily Telegraph is read by the people who think it is."

Jim Hacker, Yes Minister

Re: [Gtkpod-devel] Patches: mp4 chapters, sorting/compilation/media type tags

[Javier K. <jk...@gm...>]

On Fri, Nov 16, 2012 at 4:39 AM, Joel Smith <jsf...@jk...>wrote:

>  On 11/13/2012 02:10 AM, Javier Kohen wrote:
>
>
> On Tue, Nov 13, 2012 at 2:44 AM, Joel Smith <jsf...@jk...>wrote:
>
>> On 11/11/2012 03:09 AM, Daniele Forsi wrote:
>> > I don't know that code but in this commit
>> >
>> https://gitorious.org/~joelsmith/gtkpod/joelsmiths-sandbox/commit/6157a8c10646ff25c98651755c6dbbafa63170ba
>> >
>> > you are using g_strdup() while existing code uses charset_to_utf8()
>> >
>> > exisitng code:
>> > 182        // MP4 Title
>> > 183           value = find_atom_value(TITLE);
>> > 184           if (value) {
>> > 185               track->title = charset_to_utf8(value);
>> > 186               free(value);
>> > 187           }
>> >
>> > new code:
>> > 383        // MP4 Sort Title
>> > 384           value = find_atom_value(SORT_TITLE);
>> > 385           if (value) {
>> > 386               track->sort_title = g_strdup(value);
>> > 387               free(value);
>> > 388           }
>> >
>> > if that's ok can you please explain why with a comment in the code?
>>
>>  I think the existing code is wrong.  MP4 tags are already stored as
>> UTF-8 (or at least they're supposed to be).  And AP doesn't do any
>> further conversions before returning the strings to us, so we shouldn't
>> be trying to convert an already UTF-8 string from local charset to
>> UTF-8.  If run with a non-UTF-8 charset locale, that would corrupt the
>> data.  For example, with ISO-8859-1, all but the lower 128 char values
>> would get corrupted.  Funny that I didn't even notice the other ones
>> doing it wrong. Thanks for reviewing the patch so we could catch the
>> other bug. I'll commit a fix. I'm no expert on i18n, charsets, etc, so
>> maybe we'll end up reverting my change right away, but that's how I see
>> it.
>>
>
>  I'm no expert in i18n and Unicode either, but I have some experience. I
> can review the change if you'd like.
>
>
> The more eyes, the the better.  Here's the patch:
>
> http://gtkpod.git.sourceforge.net/git/gitweb.cgi?p=gtkpod/gtkpod;a=commitdiff;h=0fd91b5fe7e93e5b2409b7b5a36e9dd052042237
>
> My reasoning is that find_atom_value() calls APar_ExtractDataAtom() which
> for text types, simply seeks to an offset in the MP4 file and reads the
> atom value data into the return buffer, without doing any kind of charset
> conversion.  Since MP4 atom text values are supposed to be stored as UTF-8,
> we should just copy these UTF-8 strings directly, rather than calling
> charset_to_utf8() on them.
>

Sounds good to me. Here's what the official
site<http://atomicparsley.sourceforge.net/>says on this topic:

> With the exception of artwork, all iTunes-style tags support only 1 piece
> of metadata - you can't have for example 3 title tags. 3gp assets and
> copyright notices support multiple tags, but they must differ in the
> language setting for that tag. You can have for example 3 title tags, but 1
> in english, 1 in spanish, 1 in undefined. *All iTunes-style text metadata
> is always in UTF-8*; 3gp assets & copyright notices can be in set in
> UTF-8 or UTF-16. *All strings are converted internally to UTF-8*,
> converting as necessary. On *nix platforms, input is in UTF-8; the native
> Windows port supports full UTF-16 input.

Re: [Gtkpod-devel] Patches: mp4 chapters, sorting/compilation/media type tags

[Joel S. <jsf...@jk...>]

On 11/13/2012 02:10 AM, Javier Kohen wrote:
>
> On Tue, Nov 13, 2012 at 2:44 AM, Joel Smith <jsf...@jk... 
> <mailto:jsf...@jk...>> wrote:
>
>     On 11/11/2012 03:09 AM, Daniele Forsi wrote:
>     > I don't know that code but in this commit
>     >
>     https://gitorious.org/~joelsmith/gtkpod/joelsmiths-sandbox/commit/6157a8c10646ff25c98651755c6dbbafa63170ba
>     <https://gitorious.org/%7Ejoelsmith/gtkpod/joelsmiths-sandbox/commit/6157a8c10646ff25c98651755c6dbbafa63170ba>
>     >
>     > you are using g_strdup() while existing code uses charset_to_utf8()
>     >
>     > exisitng code:
>     > 182        // MP4 Title
>     > 183           value = find_atom_value(TITLE);
>     > 184           if (value) {
>     > 185               track->title = charset_to_utf8(value);
>     > 186               free(value);
>     > 187           }
>     >
>     > new code:
>     > 383        // MP4 Sort Title
>     > 384           value = find_atom_value(SORT_TITLE);
>     > 385           if (value) {
>     > 386               track->sort_title = g_strdup(value);
>     > 387               free(value);
>     > 388           }
>     >
>     > if that's ok can you please explain why with a comment in the code?
>
>     I think the existing code is wrong.  MP4 tags are already stored as
>     UTF-8 (or at least they're supposed to be).  And AP doesn't do any
>     further conversions before returning the strings to us, so we
>     shouldn't
>     be trying to convert an already UTF-8 string from local charset to
>     UTF-8.  If run with a non-UTF-8 charset locale, that would corrupt the
>     data.  For example, with ISO-8859-1, all but the lower 128 char values
>     would get corrupted.  Funny that I didn't even notice the other ones
>     doing it wrong. Thanks for reviewing the patch so we could catch the
>     other bug. I'll commit a fix. I'm no expert on i18n, charsets, etc, so
>     maybe we'll end up reverting my change right away, but that's how
>     I see it.
>
>
> I'm no expert in i18n and Unicode either, but I have some experience. 
> I can review the change if you'd like.
>

The more eyes, the the better.  Here's the patch:
http://gtkpod.git.sourceforge.net/git/gitweb.cgi?p=gtkpod/gtkpod;a=commitdiff;h=0fd91b5fe7e93e5b2409b7b5a36e9dd052042237

My reasoning is that find_atom_value() calls APar_ExtractDataAtom() 
which for text types, simply seeks to an offset in the MP4 file and 
reads the atom value data into the return buffer, without doing any kind 
of charset conversion.  Since MP4 atom text values are supposed to be 
stored as UTF-8, we should just copy these UTF-8 strings directly, 
rather than calling charset_to_utf8() on them.
Regards,
Joel

[Gtkpod-devel] iPhone 5 changes

[Jan K. <JKu...@Ja...>]

Hi all,

has someone discovered something about the new iPhone stuff? As far as I 
can see the naming scheme for artwork files has changed - and seems (on 
a first view) not to be related to anything inside the iTunesCDB or 
MediaLibrary.sqlitedb.

If I can help with some files or anything else from the iPhone, or if 
there is somewhere a better place to discuss those things - please let 
me know.

Kind Regards,

Jan.

------------------------------
JakPod - the iPod-manager
http://www.jakpod.de

Re: [Gtkpod-devel] Patches: mp4 chapters, sorting/compilation/media type tags

[Javier K. <jk...@gm...>]

On Tue, Nov 13, 2012 at 2:44 AM, Joel Smith <jsf...@jk...>wrote:

> On 11/11/2012 03:09 AM, Daniele Forsi wrote:
> > I don't know that code but in this commit
> >
> https://gitorious.org/~joelsmith/gtkpod/joelsmiths-sandbox/commit/6157a8c10646ff25c98651755c6dbbafa63170ba
> >
> > you are using g_strdup() while existing code uses charset_to_utf8()
> >
> > exisitng code:
> > 182        // MP4 Title
> > 183           value = find_atom_value(TITLE);
> > 184           if (value) {
> > 185               track->title = charset_to_utf8(value);
> > 186               free(value);
> > 187           }
> >
> > new code:
> > 383        // MP4 Sort Title
> > 384           value = find_atom_value(SORT_TITLE);
> > 385           if (value) {
> > 386               track->sort_title = g_strdup(value);
> > 387               free(value);
> > 388           }
> >
> > if that's ok can you please explain why with a comment in the code?
>
> I think the existing code is wrong.  MP4 tags are already stored as
> UTF-8 (or at least they're supposed to be).  And AP doesn't do any
> further conversions before returning the strings to us, so we shouldn't
> be trying to convert an already UTF-8 string from local charset to
> UTF-8.  If run with a non-UTF-8 charset locale, that would corrupt the
> data.  For example, with ISO-8859-1, all but the lower 128 char values
> would get corrupted.  Funny that I didn't even notice the other ones
> doing it wrong. Thanks for reviewing the patch so we could catch the
> other bug. I'll commit a fix. I'm no expert on i18n, charsets, etc, so
> maybe we'll end up reverting my change right away, but that's how I see it.
>

I'm no expert in i18n and Unicode either, but I have some experience. I can
review the change if you'd like.

Re: [Gtkpod-devel] Patches: mp4 chapters, sorting/compilation/media type tags

[Joel S. <jsf...@jk...>]

On 11/11/2012 03:09 AM, Daniele Forsi wrote:
> I don't know that code but in this commit
> https://gitorious.org/~joelsmith/gtkpod/joelsmiths-sandbox/commit/6157a8c10646ff25c98651755c6dbbafa63170ba
>
> you are using g_strdup() while existing code uses charset_to_utf8()
>
> exisitng code:
> 182        // MP4 Title
> 183	        value = find_atom_value(TITLE);
> 184	        if (value) {
> 185	            track->title = charset_to_utf8(value);
> 186	            free(value);
> 187	        }
>
> new code:
> 383        // MP4 Sort Title
> 384	        value = find_atom_value(SORT_TITLE);
> 385	        if (value) {
> 386	            track->sort_title = g_strdup(value);
> 387	            free(value);
> 388	        }
>
> if that's ok can you please explain why with a comment in the code?

I think the existing code is wrong.  MP4 tags are already stored as 
UTF-8 (or at least they're supposed to be).  And AP doesn't do any 
further conversions before returning the strings to us, so we shouldn't 
be trying to convert an already UTF-8 string from local charset to 
UTF-8.  If run with a non-UTF-8 charset locale, that would corrupt the 
data.  For example, with ISO-8859-1, all but the lower 128 char values 
would get corrupted.  Funny that I didn't even notice the other ones 
doing it wrong. Thanks for reviewing the patch so we could catch the 
other bug. I'll commit a fix. I'm no expert on i18n, charsets, etc, so 
maybe we'll end up reverting my change right away, but that's how I see it.
Regards,
Joel

Re: [Gtkpod-devel] [gtpd-cvs] [gtkpod] add mp4 chapter reading support to Atomic Parsley bridge

[Joel S. <jsf...@jk...>]

On 11/11/2012 03:26 AM, Daniele Forsi wrote:
> can we make that title translatable?
> title = g_strdup_printf(_("Chapter %3d"), i);
> and since translators will see the comments above that line, I'd
> change them a bit, something like:
> // Translators: this string is used to create a title when chapter
> title couldn't be found
> // (some iPods don't display them anyway)
>
> is this related to a specific media or it could be anything (music,
> movies, audiobooks)?
> Not a big deal, but in Italian I would translate "chapter" differently
> when it is related to music WRT other types

The chapters are not specific to any type of media. I expect that 
they're most common with movies and audiobooks, but you can put them on 
any audio or video track.

With that in mind, it isn't terribly important anyways... this code will 
only get hit if somebody has a malformed chapter in their MP4 atom 
data.  Basically, there are 3 parts that we need in order to get the 
chapter info.  But only the first one is crucial (the chapter 
duration).  The other two are the strlen of the chapter name and the 
seek offset for the chapter name.  It's only if one or both of those two 
are missing that we try to invent our own chapter names.  But I've never 
actually seen an MP4 file with broken atom data such that this code 
would get used. It's only there to keep us from getting crashy if we do 
run into such a file.

I'll make the suggested change and commit it.
Regards,
Joel

Re: [Gtkpod-devel] [gtpd-cvs] [gtkpod] add mp4 chapter reading support to Atomic Parsley bridge

[Daniele F. <df...@gm...>]

2012/11/11 Joel Smith:

> commit 3fbdec07724f4806f88dd93ac3927162b2cdaabd
> Author: Joel Smith <jsf...@jk...>
> Date:   Thu Nov 8 14:23:35 2012 -0700
>
>     add mp4 chapter reading support to Atomic Parsley bridge
>
>  libs/atomic-parsley/AtomicParsleyBridge.cpp |  125 +++++++++++++++++++++++++--
>  1 files changed, 119 insertions(+), 6 deletions(-)
> ---
> diff --git a/libs/atomic-parsley/AtomicParsleyBridge.cpp b/libs/atomic-parsley/AtomicParsleyBridge.cpp
> index ef1a182..704fd3b 100644
> --- a/libs/atomic-parsley/AtomicParsleyBridge.cpp
> +++ b/libs/atomic-parsley/AtomicParsleyBridge.cpp

> +                {
> +                    // chapter title couldn't be found; create our own titles
> +                    // (and some ipods don't display them anyway)
> +                    title = g_strdup_printf("Chapter %3d", i);
> +                }

can we make that title translatable?
title = g_strdup_printf(_("Chapter %3d"), i);
and since translators will see the comments above that line, I'd
change them a bit, something like:
// Translators: this string is used to create a title when chapter
title couldn't be found
// (some iPods don't display them anyway)

is this related to a specific media or it could be anything (music,
movies, audiobooks)?
Not a big deal, but in Italian I would translate "chapter" differently
when it is related to music WRT other types
-- 
Daniele Forsi

Re: [Gtkpod-devel] Patches: mp4 chapters, sorting/compilation/media type tags

[Daniele F. <df...@gm...>]

2012/11/9 Joel Smith:

> I have added some patches to the Atomic Parsley wrapper to get support
> for reading MP4 Quicktime-style chapters and reading/writing some
> missing tags, such as the sorting tags.

I don't know that code but in this commit
https://gitorious.org/~joelsmith/gtkpod/joelsmiths-sandbox/commit/6157a8c10646ff25c98651755c6dbbafa63170ba

you are using g_strdup() while existing code uses charset_to_utf8()

exisitng code:
182        // MP4 Title
183	        value = find_atom_value(TITLE);
184	        if (value) {
185	            track->title = charset_to_utf8(value);
186	            free(value);
187	        }

new code:
383        // MP4 Sort Title
384	        value = find_atom_value(SORT_TITLE);
385	        if (value) {
386	            track->sort_title = g_strdup(value);
387	            free(value);
388	        }

if that's ok can you please explain why with a comment in the code?
-- 
Daniele Forsi

Re: [Gtkpod-devel] Patches: mp4 chapters, sorting/compilation/media type tags

[Joel S. <jsf...@jk...>]

On 11/10/2012 02:34 PM, phantomjinx wrote:
> Hi Joel,
>
> When you are ready can you push your patches onto sf. I would like to roll them into the 2.1.3 release.
>
> Any idea whether your patches would solve [1]?
>
> Cheers
>
> PGR
>
> [1] https://sourceforge.net/tracker/index.php?func=detail&aid=3575205&group_id=67873&atid=519273
>
>

Hi PGR,
I had forgotten that I had commit access on sf. I'll push soon. 
Regarding [1] above, I had tested all the tags and not found any 
problems other than the ones I fixed, but after reading the bug report, 
I looked into it.  It looks like that's an Atomic Parseley bug where if 
you have a tempo above 99 BPM,  it won't fit into the malloc'd buffer 
that sprintf() is using for its output. (The buffer is size 4 and after 
the \n and the \0, any tempo of more than 2 digits will overflow the 
buffer). I'll make a fix and push that too.
Regards,
Joel

Re: [Gtkpod-devel] Patches: mp4 chapters, sorting/compilation/media type tags

[phantomjinx <p.g...@ph...>]

On 11/09/2012 04:29 AM, Joel Smith wrote:
> Hi all,
> I have added some patches to the Atomic Parsley wrapper to get support 
> for reading MP4 Quicktime-style chapters and reading/writing some 
> missing tags, such as the sorting tags.
> 
> Additionally, I have some minor bugfixes for reading the compilation 
> flag (which should fix 
> http://gtkpod.org/bugs/index.php?do=details&task_id=42 I think) and 
> reading the media type tag from MP4 files.
> 
> I have tested the code with my MP4 audiobooks, but most of my files are 
> more-or-less the same in terms of atom layout, etc, so if anybody has 
> some files that break my code, I'd be happy to take a look and fix the 
> code to accommodate other variations.
> 
> You can find the patches in my sandbox here:
> https://gitorious.org/~joelsmith/gtkpod/joelsmiths-sandbox/commits/master
> There are 5 patches, all pushed 2012/11/09 @ 05:08 UTC.
> 
> Regards,
> Joel

Hi Joel,

When you are ready can you push your patches onto sf. I would like to roll them into the 2.1.3 release.

Any idea whether your patches would solve [1]?

Cheers

PGR

[1] https://sourceforge.net/tracker/index.php?func=detail&aid=3575205&group_id=67873&atid=519273


-- 
Paul Richardson

  * p.g...@ph...
  * p.g...@re...
  * pgr...@li...

"I know exactly who reads the papers ...

  * The Daily Mirror is read by people who think they run the country.
  * The Guardian is read by people who think they ought to run the country.
  * The Times is read by people who do actually run the country.
  * The Daily Mail is read by the wives of the people who run the country.
  * The Financial Times is read by the people who own the country.
  * The Morning Star is read by the people who think the country ought to be run by another country.
  * The Daily Telegraph is read by the people who think it is."

Jim Hacker, Yes Minister