GNOME Bugzilla – Bug 740058
pngparse: improve parsing of the image
Last modified: 2015-04-21 06:13:28 UTC
Everytime a buffer is being provided from baseparse, we are parsing all the data from the beginning. But since we would have already parsed some of the data in the previous iterations, it doesnt make much sense to keep parsing the same everytime. Hence skipping the data which is already read in previous iterations to improve the parsing performance. On testing with the sample image MARBLE24.PNG(2.1 MB) @ http://www.fileformat.info/format/png/sample/index.htm The data chunks being read using the for loop is being called 4610 times with the present implementation. On applying my patch it will be reduced to 518 times.
Created attachment 290620 [details] [review] pngparse improvements
Looks good to me. Could you please change the commit message so no line is longer than 80 characters?
Comment on attachment 290620 [details] [review] pngparse improvements Does not look good to me. Without even looking at the other changes, this you can't do: >- guint width = 0, height = 0; >+ static guint width = 0, height = 0; or you get problems when running multiple pngparse elements, and you also can't reset these properly, etc.
Created attachment 290955 [details] [review] pngparse improvements Removed static declaration of width and height and added new logic to check for change in width and height. Please verify if this is proper.
Comment on attachment 290955 [details] [review] pngparse improvements > [...] >diff --git a/gst/videoparsers/gstpngparse.h b/gst/videoparsers/gstpngparse.h >index 3574481..2e8f234 100644 >--- a/gst/videoparsers/gstpngparse.h >+++ b/gst/videoparsers/gstpngparse.h >@@ -49,6 +49,8 @@ struct _GstPngParse > > guint width; > guint height; wouldn't hurt making this explicitly guint32 to match the spec? I understand PNG uses 32 bits unsigned integers for these values. Just a nit anyway. >+ guint32 skip_length; Coming from the above I'd make this guint64 or, unless my understanding is wrong, you risk overflowing at reading perfectly valid (albeit rather big) png frames.
> > guint width; > > guint height; > > wouldn't hurt making this explicitly guint32 to match the > spec? I understand PNG uses 32 bits unsigned integers for > these values. Just a nit anyway. Rule #1 of patch club: don't change existing things that don't need changing, keep the changeset as small as possible :)
Should i change anything with respect to the patch? as per Tims' comment, there is no need to change.
No change to width & height needed (I actually mistakenly read those as part of the changeset) but going with guint64 for skip_length would still be better choice IMHO.
Created attachment 291609 [details] [review] pngparse improvements Changing skip_length to unit64
can someone check this and commit the patch as it is already reviewed :)