[Gtkpod-devel] Re: [gtkpod-cvs] libgpod/src db-artwork-debug.c,1.1,1.2 db-artwork-debug.h,1.1,1.2 db-artwork-parser.c,1.4,1.5 db-artwork-writer.c,1.3,1.4 db-image-parser.c,1.3,1.4 db-itunes-parser.h,1.1,1.2

[Christophe F. <te...@gn...>]

Hi,=20

I was planning to review your changes and to commit bits of them, but it
looks like you've done it. Here are a few comments, including an
important one about a regression you introduced when splitting
image_dimensions in image_width and image_height. I haven't looked yet
at the ithmb-writer changes.
Can you resend a diff of your photo album changes against CVS with these
changes ?

Thanks for all your work,

Christophe



Le lundi 10 octobre 2005 =C3=A0 13:41 -0700, fst...@us...urceforge.n=
et
a =C3=A9crit :


> +	/* FIXME: do I need to translate UTF-8 to local encoding? */
> +	g_print ("\tString: \"%.*s\"\n", GINT_FROM_LE (mhod1->string_len), mhod=
1->string);

glib should do the right thing here I think, anyway it's debugging code
disabled by default, so it doesn't matter that much if we get garbage on
the console.
> +
> =20
>  		/* No information useful to us in mhod type 3, do not parse
>  		 * it in non-debug mode=20
> +		 * FIXME: really? it contains the thumbnail file name!
> +		 * we infer it from the correlation id, but is this
> +		 * always The Right Thing to do?
>  		 */

This information can be inferred from the correlation ID, that's why I
decided that information wasn't that useful. We can at least check the
filename inferred from the correlation ID is the same as the filename,
and use the filename and outputs a warning if they are different.

> @@ -304,6 +373,10 @@
>  	dump_mhfd (mhfd);
>  	cur_pos =3D ctx->header_len;
> =20
> +	/* The mhfd record always has 3 mhsd children, so it's hardcoded here.
> +	 * It could be replaced with a loop using the nb_children field from
> +	 * the mhfd record. [explanation by Christophe]
> +	 */

Would have been nice to change it to a loop instead of adding a
comment ;)


>  struct _MhodHeaderArtworkType3 {
>  	unsigned char header_id[4];
>  	gint32 header_len;
> @@ -507,7 +534,8 @@
>  	gint32 ithmb_offset;
>  	gint32 image_size;
>  	gint32 unknown3;
> -	gint32 image_dimensions;
> +	gint16 image_height;
> +	gint16 image_width;

Please revert this change, this is broken on either big endian machines,
or little endian machines, but it won't work on both.