GNOME Bugzilla – Bug 743908
simplevideomark: crash when launch with max property values
Last modified: 2015-06-05 12:16:14 UTC
When simplevideomark and simplevideomarkdetect are launched with different values of properties, then it crashes. Sample launch pipeline is as below gst-launch-1.0 videotestsrc num-buffers=20 ! simplevideomark left-offset=2147483647 ! videoconvert ! autovideosink Handling of offsets greater than video width/height is not done properly.
Created attachment 296270 [details] [review] proposed patch proposed patch fixing the issue.
Review of attachment 296270 [details] [review]: Hi, This way of fixing wont solve the actual issue. Instead of throwing error, when req_width and height are more than video width and height, we should recalculate the actual drawable width and height and display the partially visible video mark, in case half the width/height are outside video width and height, or not show the videomark in case it exists completely outside the video width and height. You can refer to how textoverlay is handled to see the expected behavior.
Created attachment 296668 [details] [review] fix display logic of videomark This is dependant on bug 744371
Review of attachment 296668 [details] [review]: gstsimplevideomarkdetect needs update as well!
Created attachment 297259 [details] [review] fix detect logic of videomark
Review of attachment 296668 [details] [review]: update patch for gstsimplevideomarkdetect
gst-launch-1.0 videotestsrc num-buffers=200 ! simplevideomark ! queue ! autovideosink After your patch the pipeline above doesn't have the video marks drawn. The default case should work as it did before. Also, perhaps it would be good to try to draw as much as possible instead of aborting all drawing when it doesn't fit the video.
Created attachment 297352 [details] [review] fix display logic of videomark Made changes such that the calculations are done using the x and y position of the mark. This helps in eliminating problems due to row stride changes across different systems. Now the logic is such that, even if the mark can be displayed partially, then it is drawn. Please check if the logic is fine. Once this is accepted, i will apply the same logic to simplevideomarkdetect.
Created attachment 298377 [details] [review] fix display logic of videomark Updating the patch with comments for easy understanding
Created attachment 298685 [details] [review] fix display logic of videomark Removing duplicate code and making sure we move to the next pattern when the pattern width is 0.
Can you please check this.
Created attachment 304397 [details] [review] fix display logic In case of XVImageSink pixel stride will be 2 and row stride will be 640 while the same in XImageSink is 1 and 320. While calculating X position we should not use pixel stride, and while moving through the data we should use pixel stride values. Please verify.
Attachment 304397 [details] included 3 lines with trailing white spaces, please use gst-indent to avoid these in future contributions. Also, please mark that a patch obsoletes or replaces an other when submitting. If not it looks like attachment 304397 [details] [review] goes after applying 297259. Tested your code. Now if the mark is outside the frame is not displayed and it doesn't crash, which is what I believe is the expected behaviour. Merging
Review of attachment 297259 [details] [review]: Replaced by 304397
Review of attachment 304397 [details] [review]: Commit 665f2f1afc3ab2a39e47e1965af0a4da39981581
Created attachment 304566 [details] [review] fix detect logic Luis thanks for the review :) The other patch is in SimpleVideoMarkDetect element. Which is similar but used to detect the mark instead of displaying. The changes are almost similar. I thought i will make changes in that after this get accepted.. Now i have made same changes in detect element as well :) Please review
Hi Vineeth, I was discussing this with Thiago and he convinced me that the merged patch should be fixed so it still sends an ERROR notifying that the offset is off the screen. Could you please do this before I review the new patch?
Created attachment 304621 [details] [review] Added error log if pattern is outside the video
Created attachment 304622 [details] [review] fix detect logic Added error log to this as well. This can be tested with the below pipeline gst-launch-1.0 videotestsrc num-buffers=20 ! simplevideomark ! simplevideomarkdetect ! videoconvert ! autovideosink This can be verified by printing simplevideomarkdetect->in_pattern. And testing for various offsets. If the offset values match in simplevideomark and simplevideomarkdetect then the in_pattern value should be TRUE and FALSE otherwise. Please review.
Review of attachment 304621 [details] [review]: commit abed8af00c4aa7f7367b5b3b2b093329b75c8693 Author: Vineeth TM <vineeth.tm@samsung.com> Date: Fri Jun 5 08:53:30 2015 +0900 simplevideomark: Add Error logs When the pattern offset is outside the video, the print error message https://bugzilla.gnome.org/show_bug.cgi?id=743908
Review of attachment 304621 [details] [review]: commit 7824f4cf52dbc6221022edec2ed7402de72c993e Author: Vineeth TM <vineeth.tm@samsung.com> Date: Fri Jun 5 08:58:03 2015 +0900 simplevideomarkdetect: fix detect of videomark partially or fully outside video In case of the videomark being partially or fully outside, an error was bein thrown saying, mark width is more than video width. And when the width, offset properties are set to maximum it resulted in crash. Instead of throwing error, added logic to detect the mark in case of partial visibility or dont show the mark when it is outside. https://bugzilla.gnome.org/show_bug.cgi?id=743908
Thanks Vineeth!