GNOME Bugzilla – Bug 737984
schrodec: optimize parse logic
Last modified: 2014-10-10 12:07:31 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.
Created attachment 287828 [details] [review] optimize schrodec parse logic
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?
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 http://diracvideo.org/download/test-streams/raw/vts/vts.profile-vc2-low-delay.drc 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.
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. http://samples.libav.org/V-codecs/Dirac/src3_422p_ffdirac.drc
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) { return GST_VIDEO_DECODER_FLOW_NEED_DATA; } else schro_decoder->header_read = FALSE; Regards, Vineeth
I think it would be easier to read if you have it in an else clause, yes.
Created attachment 288087 [details] [review] Update the patch as per review comments
Thanks Vineeth!
Comment on attachment 288087 [details] [review] Update the patch as per review comments Merged