Bug 89673 - nicesrc: Some minor cleanups
Summary: nicesrc: Some minor cleanups
Status: RESOLVED FIXED
Alias: None
Product: nice
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium minor
Assignee: Olivier Crête
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-19 09:37 UTC by Sebastian Dröge (slomo)
Modified: 2015-04-20 21:39 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
nicesrc: Keep the mutex locked whenever accessing the queue (1.36 KB, patch)
2015-03-19 09:37 UTC, Sebastian Dröge (slomo)
Details | Splinter Review
nicesrc: Attach the receive callback in READY->PAUSED and detach in PAUSED->READY (2.20 KB, patch)
2015-03-19 09:37 UTC, Sebastian Dröge (slomo)
Details | Splinter Review
nicesrc: Clear the output buffer queue when shutting down and disposing (1.41 KB, patch)
2015-03-19 09:37 UTC, Sebastian Dröge (slomo)
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Dröge (slomo) 2015-03-19 09:37:10 UTC
See attached patches
Comment 1 Sebastian Dröge (slomo) 2015-03-19 09:37:41 UTC
Created attachment 114465 [details] [review]
nicesrc: Keep the mutex locked whenever accessing the queue
Comment 2 Sebastian Dröge (slomo) 2015-03-19 09:37:45 UTC
Created attachment 114466 [details] [review]
nicesrc: Attach the receive callback in READY->PAUSED and detach in PAUSED->READY

Receiving data before the element is ready to push them does not seem like the
best idea.
Comment 3 Sebastian Dröge (slomo) 2015-03-19 09:37:49 UTC
Created attachment 114467 [details] [review]
nicesrc: Clear the output buffer queue when shutting down and disposing
Comment 4 Olivier Crête 2015-03-27 23:31:48 UTC
Comment on attachment 114465 [details] [review]
nicesrc: Keep the mutex locked whenever accessing the queue

Review of attachment 114465 [details] [review]:
-----------------------------------------------------------------

I think the queue is only accessed from the streaming thread, so no need for locking.
Comment 5 Olivier Crête 2015-03-27 23:33:00 UTC
Comment on attachment 114466 [details] [review]
nicesrc: Attach the receive callback in READY->PAUSED and detach in PAUSED->READY

Review of attachment 114466 [details] [review]:
-----------------------------------------------------------------

The data is received from the gmainloop ran in the streaming thread, so it shouldn't be a problem.
Comment 6 Olivier Crête 2015-03-27 23:37:29 UTC
Comment on attachment 114467 [details] [review]
nicesrc: Clear the output buffer queue when shutting down and disposing

Review of attachment 114467 [details] [review]:
-----------------------------------------------------------------

Pushed this one

Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Mar 19 09:21:23 2015 +0100

    nicesrc: Clear the output buffer queue when shutting down and disposing
    
    https://bugs.freedesktop.org/show_bug.cgi?id=89673
Comment 7 Sebastian Dröge (slomo) 2015-04-13 09:43:45 UTC
(In reply to Olivier Crête from comment #4)
> Comment on attachment 114465 [details] [review] [review]
> nicesrc: Keep the mutex locked whenever accessing the queue
> 
> Review of attachment 114465 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I think the queue is only accessed from the streaming thread, so no need for
> locking.

It's also accessed from unlock(), which is called from any thread


(In reply to Olivier Crête from comment #5)
> Comment on attachment 114466 [details] [review] [review]
> nicesrc: Attach the receive callback in READY->PAUSED and detach in
> PAUSED->READY
> 
> Review of attachment 114466 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> The data is received from the gmainloop ran in the streaming thread, so it
> shouldn't be a problem.

But attaching the callback will let libnice start doing its stuff and trying to establish the connection. Seems wrong to start with that in READY already
Comment 8 Olivier Crête 2015-04-20 21:39:19 UTC
Arg, you're right, sorry, patches merged

commit e3860ddfb5667de8f3f9be27838ea7e4152faf01
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Mar 19 09:19:23 2015 +0100

    nicesrc: Attach the receive callback in READY->PAUSED and detach in PAUSED->READY
    
    Receiving data before the element is ready to push them does not seem like the
    best idea.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=89673

commit 34d03c15a14e9ea46ef2f3201f12b90755154296
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Mar 19 09:17:38 2015 +0100

    nicesrc: Keep the mutex locked whenever accessing the queue
    
    https://bugs.freedesktop.org/show_bug.cgi?id=89673


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.