GNOME Bugzilla – Bug 737708
pngdec: change parse logic
Last modified: 2014-11-04 11:01:08 UTC
Right now in parse logic, the signature is checked every time the parse function is called, and the whole data is the scanned each and every time the parse function is called, even though the data is scanned in the previous instance of parse being called. Changing the logic such that, we skip the bytes which is already scanned in the previous instances of parse. This helps in avoiding multiple scan of already scanned data/signature. Please check if this is proper.
Created attachment 287505 [details] [review] Change pngdec parse logic
Review of attachment 287505 [details] [review]: ::: ext/libpng/gstpngdec.c @@ +113,2 @@ pngdec->image_ready = FALSE; + pngdec->read_data = 0; You have to reset it in ::stop() and ::flush() too
Created attachment 287790 [details] [review] Change pngdec parse logic resetting read_data in clear function, which will be called in both ::flush and ::stop
Thanks for the review Sebastian :)
ping.
Vineeth, we are all a bit busy with the GStreamer Conference coming up next week. Patience :)
ping :)...
Vineeth, looks good to me. Do you mind sharing the pipeline you are using to test this, want to test it fully. Just curious, why do increment read_data by 12? Is the skip by 4 that happens above too big? + pngdec->read_data += length + 12;
Hi Luis, Thanks for the review. I am using a simple pipeline to test the same gst-launch-1.0 filesrc location=../media/icon.png ! pngdec ! videoconvert ! ximagesink And the value 12 is because of below if (!gst_byte_reader_get_uint32_be (&reader, &length)) goto need_more_data; if (!gst_byte_reader_get_uint32_le (&reader, &code)) goto need_more_data; if (!gst_byte_reader_skip (&reader, length + 4)) goto need_more_data; 4 for uint32_be, 4 for uint32_le and length + 4 for skip so total of length + 12
Hi Vineeth, Could you remove the changes you have done in the header file (gstpngdec.h) except adding the read_data variable? You do a few style changes which shouldn't be made. You also remove a line from gst_pngdec_parse() in the C file. Please remove that style change. Everything else looks good and once you have updated the patch removing these style changes I will merge. Thanks :)
Created attachment 289860 [details] [review] Change pngdec parse logic Updated by removing style changes. Please review
One more left :P > @@ -468,7 +473,6 @@ gst_pngdec_parse (GstVideoDecoder * decoder, GstVideoCodecFrame * frame, > goto need_more_data; > if (!gst_byte_reader_get_uint32_le (&reader, &code)) > goto need_more_data; > -
Created attachment 289957 [details] [review] Change pngdec parse logic hopefully it is fine now :)
Fixed the formatting of the commit message and merged! Vineeth, there is no need for indentation in the commit message. You are limited to 80 characters in the width of the body so make the most of them. commit 63e0b292910d51d5e948d25a8aa13b6e73a12278 Author: Vineeth T M <vineeth.tm@samsung.com> Date: Tue Nov 4 08:18:41 2014 +0530 pngdec: change parse logic Right now in parse logic the signature is checked every time the parse function is called, and the whole data is the scanned each and every time, even though the data is scanned in the previous instance. Changing the logic such that, we skip the bytes which are already scanned in the previous instances of parse. This helps in avoiding multiple scan of already scanned data/signature. https://bugzilla.gnome.org/show_bug.cgi?id=737708
Comment on attachment 289957 [details] [review] Change pngdec parse logic Thanks Vineeth!