Re: [Gegl-developer] Here's a git patch for operations/external/png-load.c
On 11/24/2010 12:58 PM, Martin Nordholts wrote: > On 11/23/2010 05:00 AM, Patrick Horgan wrote: >> On 11/16/2010 11:22 AM, Martin Nordholts wrote: >>> Hi >>> >>> The patch doesn't apply cleanly for me (sorry for messy formating): >> Sorry about that. I think I may know why, I was two checkins away from >> trunk and the patch only reflected one. This one should apply, but if >> not please let me know. >> >> Patrick > > Could you add this to bugzilla please? I don't think I'll have time to > look into the patch in a few weeks in worst case. > > I know it's not a big patch, but I need to collect some motivation too. Ok:) I've gotten a patch into GIMP before, but this would be my first in gegl. https://bugzilla.gnome.org/show_bug.cgi?id=635747 Patrick ___ Gegl-developer mailing list [email protected] https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
Re: [Gegl-developer] Here's a git patch for operations/external/png-load.c
On 11/16/2010 11:22 AM, Martin Nordholts wrote:
Hi
The patch doesn't apply cleanly for me (sorry for
messy formating):
Sorry about that. I think I may know why, I was two
checkins away from trunk and the patch only reflected
one. This one should apply, but if not please let me know.
Patrick
>From c9192c9086539a6840e4f3cc90ddd71df1a21ec6 Mon Sep 17 00:00:00 2001
From: Patrick Horgan
Date: Fri, 19 Nov 2010 18:31:34 -0800
Subject: [PATCH] Check fread return status for error so that if the file is short, then
will be able to give a more meaningful error. Also abstract current
code out of two routines that open the file and read it.
---
operations/external/png-load.c | 68
1 files changed, 41 insertions(+), 27 deletions(-)
diff --git a/operations/external/png-load.c b/operations/external/png-load.c
index 9f0d210..b0a527f 100644
--- a/operations/external/png-load.c
+++ b/operations/external/png-load.c
@@ -34,6 +34,43 @@ gegl_chant_file_path (path, _("File"), "", _("Path of file to load."))
#include "gegl-chant.h"
#include
+static FILE * open_png(const gchar *path)
+{
+ FILE *infile;
+ const size_t hdr_size=8;
+ size_t hdr_read_size;
+ unsigned char header[hdr_size];
+
+ if (!strcmp (path, "-"))
+{
+ infile = stdin;
+}
+ else
+{
+ infile = fopen (path, "rb");
+}
+ if (!infile)
+{
+ return infile;
+}
+
+ if((hdr_read_size=fread(header, 1, hdr_size, infile))!=hdr_size)
+{
+ fclose(infile);
+ g_warning ("%s is too short for a png file, only %d bytes.",
+ path, hdr_read_size);
+ return NULL;
+}
+
+ if (png_sig_cmp (header, 0, hdr_size))
+{
+ fclose (infile);
+ g_warning ("%s is not a png file", path);
+ return NULL;
+}
+ return infile;
+}
+
static gint
gegl_buffer_import_png (GeglBuffer *gegl_buffer,
const gchar *path,
@@ -53,7 +90,6 @@ gegl_buffer_import_png (GeglBuffer *gegl_buffer,
FILE *infile;
png_structpload_png_ptr;
png_infop load_info_ptr;
- unsigned char header[8];
guchar*pixels;
/*png_bytep *rows;*/
@@ -61,24 +97,10 @@ gegl_buffer_import_png (GeglBuffer *gegl_buffer,
unsigned int i;
png_bytep *row_p = NULL;
- if (!strcmp (path, "-"))
-{
- infile = stdin;
-}
- else
-{
- infile = fopen (path, "rb");
-}
- if (!infile)
-{
- return -1;
-}
+ infile = open_png (path);
- fread (header, 1, 8, infile);
- if (png_sig_cmp (header, 0, 8))
+ if (!infile)
{
- fclose (infile);
- g_warning ("%s is not a png file", path);
return -1;
}
@@ -239,21 +261,13 @@ static gint query_png (const gchar *path,
FILE *infile;
png_structp load_png_ptr;
png_infop load_info_ptr;
- unsigned char header[8];
png_bytep *row_p = NULL;
- infile = fopen (path, "rb");
- if (!infile)
-{
- return -1;
-}
+ infile = open_png (path);
- fread (header, 1, 8, infile);
- if (png_sig_cmp (header, 0, 8))
+ if (!infile)
{
- fclose (infile);
- g_warning ("%s is not a png file", path);
return -1;
}
--
1.7.1
___
Gegl-developer mailing list
[email protected]
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
Re: [Gegl-developer] Here's a git patch for operations/external/png-load.c
On 11/09/2010 10:32 PM, Martin Nordholts wrote:
Thanks! A few comments:
On 11/10/2010 02:02 AM, Patrick Horgan wrote:
+ if(fread (header, 1, 8, infile)!=8)
No magic numbers please, use a define for at least 8
Ok, I use a const size_t now.
+ g_warning ("%s is not a valid png file", path);
You should give a more detailed error message than just "is not valid"
Ok, now I say that it is too short to be a png file.
+ if(fread (header, 1, 8, infile)!=8)
+{
+ fclose (infile);
+ g_warning ("%s is not a valid png file", path);
+ return -1;
+}
Instead of duplicating this code from above, it would be better to put
it in a helper function
/ Martin
Ok, I abstracted the beginning code from the two
routines into a open_png(const gchar path)
routine that returns the NULL for failure just as
fopen() does, but on successful return has
advanced past the header and checked it.
It was the most that I could abstract without out args
or returning a struct, because each of the
routines begins to allocate local data structures with
libpng calls right after this. Seemed to
maximize elegance.
I just copied the indentation scheme.
patch is against origin
Patrick
>From 9801526ff68166629ed0620da30304fc2dac300e Mon Sep 17 00:00:00 2001
From: Patrick Horgan
Date: Thu, 11 Nov 2010 05:57:21 -0800
Subject: [PATCH 2/2] abstract common part of two routines to common png_open and have a short
read of header give better diagnostic
---
operations/external/png-load.c | 79 +---
1 files changed, 41 insertions(+), 38 deletions(-)
diff --git a/operations/external/png-load.c b/operations/external/png-load.c
index 44128d8..b0a527f 100644
--- a/operations/external/png-load.c
+++ b/operations/external/png-load.c
@@ -34,6 +34,43 @@ gegl_chant_file_path (path, _("File"), "", _("Path of file to load."))
#include "gegl-chant.h"
#include
+static FILE * open_png(const gchar *path)
+{
+ FILE *infile;
+ const size_t hdr_size=8;
+ size_t hdr_read_size;
+ unsigned char header[hdr_size];
+
+ if (!strcmp (path, "-"))
+{
+ infile = stdin;
+}
+ else
+{
+ infile = fopen (path, "rb");
+}
+ if (!infile)
+{
+ return infile;
+}
+
+ if((hdr_read_size=fread(header, 1, hdr_size, infile))!=hdr_size)
+{
+ fclose(infile);
+ g_warning ("%s is too short for a png file, only %d bytes.",
+ path, hdr_read_size);
+ return NULL;
+}
+
+ if (png_sig_cmp (header, 0, hdr_size))
+{
+ fclose (infile);
+ g_warning ("%s is not a png file", path);
+ return NULL;
+}
+ return infile;
+}
+
static gint
gegl_buffer_import_png (GeglBuffer *gegl_buffer,
const gchar *path,
@@ -53,7 +90,6 @@ gegl_buffer_import_png (GeglBuffer *gegl_buffer,
FILE *infile;
png_structpload_png_ptr;
png_infop load_info_ptr;
- unsigned char header[8];
guchar*pixels;
/*png_bytep *rows;*/
@@ -61,30 +97,10 @@ gegl_buffer_import_png (GeglBuffer *gegl_buffer,
unsigned int i;
png_bytep *row_p = NULL;
- if (!strcmp (path, "-"))
-{
- infile = stdin;
-}
- else
-{
- infile = fopen (path, "rb");
-}
- if (!infile)
-{
- return -1;
-}
-
- if(fread (header, 1, 8, infile)!=8)
-{
- fclose (infile);
- g_warning ("%s is not a valid png file", path);
- return -1;
-}
+ infile = open_png (path);
- if (png_sig_cmp (header, 0, 8))
+ if (!infile)
{
- fclose (infile);
- g_warning ("%s is not a png file", path);
return -1;
}
@@ -245,26 +261,13 @@ static gint query_png (const gchar *path,
FILE *infile;
png_structp load_png_ptr;
png_infop load_info_ptr;
- unsigned char header[8];
png_bytep *row_p = NULL;
- infile = fopen (path, "rb");
- if (!infile)
-{
- return -1;
-}
+ infile = open_png (path);
- if(fread (header, 1, 8, infile)!=8)
-{
- fclose (infile);
- g_warning ("%s is not a valid png file", path);
- return -1;
-}
- if (png_sig_cmp (header, 0, 8))
+ if (!infile)
{
- fclose (infile);
- g_warning ("%s is not a png file", path);
return -1;
}
--
1.7.1
___
Gegl-developer mailing list
[email protected]
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
Re: [Gegl-developer] Here's a git patch for operations/external/png-load.c
Thanks! A few comments:
On 11/10/2010 02:02 AM, Patrick Horgan wrote:
> + if(fread (header, 1, 8, infile)!=8)
No magic numbers please, use a define for at least 8
> + g_warning ("%s is not a valid png file", path);
You should give a more detailed error message than just "is not valid"
> + if(fread (header, 1, 8, infile)!=8)
> +{
> + fclose (infile);
> + g_warning ("%s is not a valid png file", path);
> + return -1;
> +}
Instead of duplicating this code from above, it would be better to put
it in a helper function
/ Martin
--
My GIMP Blog:
http://www.chromecode.com/
"Nightly GIMP, GEGL, babl tarball builds"
___
Gegl-developer mailing list
[email protected]
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
