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 737984 - schrodec: optimize parse logic
schrodec: optimize parse logic
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2014-10-06 09:57 UTC by Vineeth
Modified: 2014-10-10 12:07 UTC
See Also:
GNOME target: ---
GNOME version: ---

optimize schrodec parse logic (6.39 KB, patch)
2014-10-06 09:59 UTC, Vineeth
none Details | Review
update patch (6.40 KB, patch)
2014-10-07 05:31 UTC, Vineeth
none Details | Review
Update the patch as per review comments (6.41 KB, patch)
2014-10-09 02:52 UTC, Vineeth
committed Details | Review

Description Vineeth 2014-10-06 09:57:20 UTC
Header will be read each and everytime parse function will be called
which is not necessary since until we have complete data,
we need not parse the header again.

Added a patch to check if the header is already parsed or not, and based on the same skipping the header reading.
Please review the same.
Comment 1 Vineeth 2014-10-06 09:59:41 UTC
Created attachment 287828 [details] [review]
optimize schrodec parse logic
Comment 2 Luis de Bethencourt 2014-10-06 12:17:52 UTC
This is a good find.

I want to check this to make sure some details of the logic are right. What pipeline did you use to test this?
Comment 3 Vineeth 2014-10-07 05:31:09 UTC
Created attachment 287906 [details] [review]
update patch

had made a simple mistake. Hence updating the patch again.

i tested with the following pipeline

filesrc location=../1.drc ! schrodec ! videoconvert ! ximagesink

where the sample file is downloaded from

With the above sample file, parse function is being called 234 times, 
which means with the existing logic, the header scan will happen all 234 times.
With my updated logic to check if the header is already scanned, the scan happens only for 93 times.

PS: in my pc, i was not able to install schroedinger-1.0.11 properly.
Hence i tweaked a bit to make it work, by just compiling and adding the compiled library path to element make file.
Comment 4 Luis de Bethencourt 2014-10-08 10:58:09 UTC
I like it and it works great.

Only one question before I merge. After applying your patch in line 353 you have schro_decoder->header_read = FALSE;
What is the purpose of setting it to FALSE there? Not questioning it, just want to be sure I understand.

Playing the following file the header is scanned 231 times, instead of 806 with the master branch.
Comment 5 Vineeth 2014-10-08 11:12:10 UTC
Hi Luis,
   Thanks for the review.

   The header_read is true only when we return GST_VIDEO_DECODER_FLOW_NEED_DATA.
   We have read a header and have the number of bytes to read in 'next', but the adapter does not consist 'next' bytes, then we are asking for more data. In this scenario we will not read header again.
   In all other cases, we should set header_read to false.

   Perhaps it would make more sense if i add the same in else case?
   if (gst_adapter_available (adapter) < schro_decoder->next) {
     schro_decoder->header_read = FALSE;

Comment 6 Luis de Bethencourt 2014-10-08 13:02:33 UTC
I think it would be easier to read if you have it in an else clause, yes.
Comment 7 Vineeth 2014-10-09 02:52:09 UTC
Created attachment 288087 [details] [review]
Update the patch as per review comments
Comment 8 Luis de Bethencourt 2014-10-10 12:07:13 UTC
Thanks Vineeth!
Comment 9 Luis de Bethencourt 2014-10-10 12:07:31 UTC
Comment on attachment 288087 [details] [review]
Update the patch as per review comments