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 749531 - xvimagesink: refactor code to remove unnecessary mutex lock/unlock
xvimagesink: refactor code to remove unnecessary mutex lock/unlock
Status: RESOLVED NOTABUG
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other Linux
: Normal enhancement
: NONE
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-18 07:47 UTC by Vineeth
Modified: 2015-05-18 12:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
remove unnecessary mutex lock/unlock (1.89 KB, patch)
2015-05-18 07:48 UTC, Vineeth
rejected Details | Review

Description Vineeth 2015-05-18 07:47:14 UTC
When xvimagesink->xwindow is not present, there is a seperate condition
 to check it and prepare window handle, and mutex lock/unlock is being called
 seperately for the same. This is not needed. The same condition can be added 
 while creating the new instance of xwindow.
Comment 1 Vineeth 2015-05-18 07:48:21 UTC
Created attachment 303506 [details] [review]
remove unnecessary mutex lock/unlock
Comment 2 Tim-Philipp Müller 2015-05-18 11:26:23 UTC
I think you misunderstood what this code does.

The prepare_window_handle() will post a message on the bus that gives the application a chance to supply its own window handle. Applications might intercept the message in a sync message handler and then call the gst_video_overlay_set_window_handle().

The the whole point of this code is to first give the application a chance to set its own window, and then in the second if() we create our own if we didn't get one from the application.

Furthermore, prepare_window_handle() must be called with the lock UNLOCKED otherwise we get a deadlock if application code calls one of our functions that takes the lock again (like _set_window_handle).

Please re-open if I missed something though, thanks.
Comment 3 Vineeth 2015-05-18 12:00:23 UTC
Hmm it does make sense..

But what i didn't understand is why do we need to lock just to check if window is not null and then again unlock it..
it is not like we are operating on the window..
Or is it needed to lock even for just checking?
Comment 4 Tim-Philipp Müller 2015-05-18 12:12:40 UTC
You need to lock whilst checking as well, because it's possible that the window is just being set from the application thread at the same time.

The extra locking should not really be a problem here, the set_caps function is not called very often.