After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 143608 - Add comment option for jpeg, xbm, gif and pnm
Add comment option for jpeg, xbm, gif and pnm
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: loaders
git master
Other Linux
: Normal enhancement
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2004-06-02 20:26 UTC by Dave Neary
Modified: 2017-03-15 23:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
JPEG Comment patch (3.60 KB, patch)
2017-03-12 08:41 UTC, Pawel Xj
none Details | Review
image for test (187.37 KB, image/jpeg)
2017-03-12 08:43 UTC, Pawel Xj
  Details
JPEG Comment patch v2 (241.72 KB, patch)
2017-03-13 05:16 UTC, Pawel Xj
none Details | Review
JPEG Comment patch v3 (238.01 KB, patch)
2017-03-13 21:54 UTC, Pawel Xj
none Details | Review
jpeg: Add support for JPEG_COM EXIF tag (238.06 KB, patch)
2017-03-15 13:23 UTC, Bastien Nocera
none Details | Review
jpeg: Add support for JPEG_COM EXIF tag (238.75 KB, patch)
2017-03-15 13:32 UTC, Bastien Nocera
committed Details | Review

Description Dave Neary 2004-06-02 20:26:56 UTC
Currently the comment string for these formats is read and discarded, it would
be nice if there were a "comment" option attached to the GdkPixbuf instead. In
fact, it would be nice if the tEXt chunk from a png were also stored in the
"comment" option, for consistency.

Cheers,
Dave.
Comment 1 Sven Neumann 2004-06-03 12:05:39 UTC
PNG files don't have comments. They have tEXt or iTXt chunks and there's a
predefined set of keywords to use. "Comment" is one of them. So in order to
access the comment in a PNG, you use "tEXt::Comment".

See also http://www.libpng.org/pub/png/spec/1.1/PNG-Chunks.html#C.tEXt
Comment 2 Dave Neary 2004-06-03 12:29:47 UTC
That wouldn't prevent GdkPixbuf from setting a generic "comment" option
containing the contents of tEXt::Comment, as well as storing the tEXt::Comment
option itself. 

A generic "comment" option would have the benefit of being usable regardless of
the input and output format, where tEXt::* chunks are png specific.

Cheers,
Dave.
Comment 3 Matthias Clasen 2004-06-21 23:57:52 UTC
We two patches here, one to store the comment in an option when a image in one
of the mentioned formats is loaded, and one to add support for a "comment"
option when saving these formats. Care must be taken to properly handle the
restrictions of the image formats wrt to string lengths and encodings.
Comment 4 Pawel Xj 2017-02-18 23:45:20 UTC
Any info on this ? I'll working on display jpeg comments in XFCE/ristretto and since its using gdk pixbuf - adding this to the options will be the best approach.
Comment 5 Dave Neary 2017-02-20 00:41:19 UTC
13 years later.... I guess that this has either been resolved, or completely forgotten.

Dave.
Comment 6 Pawel Xj 2017-02-20 21:34:42 UTC
It wasn't resolved - at least not in JPEG part. I've made that code. There is pull request for it:
https://github.com/GNOME/gdk-pixbuf/pull/2
Comment 7 Pawel Xj 2017-02-21 00:59:15 UTC
Great .. very kindly bot closed my pull request and try to force me to read documentation - just for few line changes.
Can someone with access to official repository just commit my changes ?
Comment 8 Pawel Xj 2017-03-12 08:41:33 UTC
Created attachment 347739 [details] [review]
JPEG Comment patch
Comment 9 Pawel Xj 2017-03-12 08:43:47 UTC
Created attachment 347740 [details]
image for test
Comment 10 Bastien Nocera 2017-03-12 14:05:59 UTC
Review of attachment 347739 [details] [review]:

You need to use git format-patch to generate your patch, otherwise it has no authorship data, and can't store binary changes either.

Good first pass, thanks.

::: gdk-pixbuf/io-jpeg.c
@@ +521,3 @@
 		else if (cmarker->marker == JPEG_APP0+2)
 			jpeg_parse_exif_app2_segment (context, cmarker);
+

White space change, please remove.

@@ +527,3 @@
 
+static gchar*
+jpeg_get_comment ( j_decompress_ptr cinfo ) {

Please follow the coding style in the rest of the file. That should be:
static gchar *
jpeg_get_comment (j_decompress_ptr cinfo)
{
...

@@ +529,3 @@
+jpeg_get_comment ( j_decompress_ptr cinfo ) {
+	jpeg_saved_marker_ptr cmarker;
+	for ( cmarker = cinfo->marker_list;

Single line, no space after the parenthesis.

@@ +534,3 @@
+	{
+		if (cmarker->marker == JPEG_COM)
+			return g_strndup( (const gchar*) cmarker->data, cmarker->data_length );

Space before the paren, not after.

@@ +634,3 @@
 	}
 
+	gchar* comment = jpeg_get_comment( &cinfo );

Declarations at the top of the block.

::: tests/pixbuf-jpeg.c
@@ +109,3 @@
+    }
+
+  ref = gdk_pixbuf_new_from_file (g_test_get_filename (G_TEST_DIST, "bug725582-testrotate.jpg", NULL), &error);

Rather than modifying an existing image, I'd rather you create a new one. Any one would do.
Comment 11 Pawel Xj 2017-03-13 05:16:31 UTC
Created attachment 347795 [details] [review]
JPEG Comment patch v2
Comment 12 Pawel Xj 2017-03-13 05:16:57 UTC
Done.
Comment 13 Bastien Nocera 2017-03-13 13:53:53 UTC
Please squash those patches together, it's not reviewable as-is.
Comment 14 Pawel Xj 2017-03-13 21:54:34 UTC
Created attachment 347882 [details] [review]
JPEG Comment patch v3
Comment 15 Bastien Nocera 2017-03-15 13:23:17 UTC
Created attachment 348006 [details] [review]
jpeg: Add support for JPEG_COM EXIF tag
Comment 16 Bastien Nocera 2017-03-15 13:24:23 UTC
Comment on attachment 347882 [details] [review]
JPEG Comment patch v3

Still about half the lines don't follow the coding style around it. I fixed all those below.
Comment 17 Bastien Nocera 2017-03-15 13:32:06 UTC
Created attachment 348008 [details] [review]
jpeg: Add support for JPEG_COM EXIF tag
Comment 18 Bastien Nocera 2017-03-15 13:34:45 UTC
I've fixed all the coding style problems, and added documentation. I'm
a bit worried about the unbounded data copying, but I'm not certain
there's a good way to fix this. We'll see.

XBM and PNM are lost causes, and we won't add comments support for those.
For GIF, if somebody is interested, then please file a separate bug.

Attachment 348008 [details] pushed as 47c587b - jpeg: Add support for JPEG_COM EXIF tag
Comment 19 Pawel Xj 2017-03-15 23:00:08 UTC
Thanks for all your help. I believe g_strndup should take care of at least null-termination, so it should be safe.