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 737708 - pngdec: change parse logic
pngdec: change parse logic
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-10-01 10:41 UTC by Vineeth
Modified: 2014-11-04 11:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Change pngdec parse logic (5.34 KB, patch)
2014-10-01 10:42 UTC, Vineeth
needs-work Details | Review
Change pngdec parse logic (5.52 KB, patch)
2014-10-06 02:50 UTC, Vineeth
none Details | Review
Change pngdec parse logic (4.78 KB, patch)
2014-11-03 03:21 UTC, Vineeth
none Details | Review
Change pngdec parse logic (4.49 KB, patch)
2014-11-04 02:50 UTC, Vineeth
committed Details | Review

Description Vineeth 2014-10-01 10:41:16 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.
Comment 1 Vineeth 2014-10-01 10:42:59 UTC
Created attachment 287505 [details] [review]
Change pngdec parse logic
Comment 2 Sebastian Dröge (slomo) 2014-10-02 07:47:53 UTC
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
Comment 3 Vineeth 2014-10-06 02:50:50 UTC
Created attachment 287790 [details] [review]
Change pngdec parse logic

resetting read_data in clear function, which will be called in both ::flush and ::stop
Comment 4 Luis de Bethencourt 2014-10-06 12:18:20 UTC
Thanks for the review Sebastian :)
Comment 5 Vineeth 2014-10-09 02:52:28 UTC
ping.
Comment 6 Luis de Bethencourt 2014-10-10 12:08:39 UTC
Vineeth, we are all a bit busy with the GStreamer Conference coming up next week. Patience :)
Comment 7 Vineeth 2014-10-27 11:26:19 UTC
ping :)...
Comment 8 Luis de Bethencourt 2014-10-27 14:49:34 UTC
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;
Comment 9 Vineeth 2014-10-28 02:54:47 UTC
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
Comment 10 Luis de Bethencourt 2014-10-31 15:58:55 UTC
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 :)
Comment 11 Vineeth 2014-11-03 03:21:14 UTC
Created attachment 289860 [details] [review]
Change pngdec parse logic

Updated by removing style changes.
Please review
Comment 12 Luis de Bethencourt 2014-11-03 14:31:02 UTC
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;
> -
Comment 13 Vineeth 2014-11-04 02:50:55 UTC
Created attachment 289957 [details] [review]
Change pngdec parse logic

hopefully it is fine now :)
Comment 14 Luis de Bethencourt 2014-11-04 11:00:10 UTC
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 15 Luis de Bethencourt 2014-11-04 11:00:54 UTC
Comment on attachment 289957 [details] [review]
Change pngdec parse logic

Thanks Vineeth!