On Mon, 2013-08-12 at 13:51 +0200, David Henningsson wrote: > On 08/12/2013 01:34 PM, Tanu Kaskinen wrote: > > On Mon, 2013-08-12 at 13:28 +0200, David Henningsson wrote: > >> On 08/12/2013 12:18 PM, Tanu Kaskinen wrote: > >>> On Mon, 2013-08-12 at 09:15 +0200, David Henningsson wrote: > >>>> On 08/09/2013 08:57 AM, Tanu Kaskinen wrote: > >>>>> Monitor sources don't have the update_rate() callback set, so their rate was > >>>>> not being changed when changing the sink rate. > >>>> > >>>> For better understanding (I was a little confused first), one could add > >>>> a sentence to the commit comment saying e g "This patch fixes this by > >>>> changing the rate correctly, even if the update_rate callback is not set". > >>> > >>> Ok. > >>> > >>>> Can this also happen to sinks, that there might be types of sinks that > >>>> do not have update_rate set, and might fall into the same bug? > >>> > >>> I don't think there are such sinks. Monitor sources are special, because > >>> they are automatically created by the core. All other sources and sinks > >>> are supposed to set update_rate() if they support rate switching. > >>> > >> > >> So, with this patch, it looks like sources not supporting rate > >> switching, will not set update_rate initially, and then crash in > >> assert(monitor_of) ? > > > > There's > > > > - if (!s->update_rate) > > + if (!s->update_rate && !s->monitor_of) > > return false; > > > > in the beginning of the patch. > > > > Oh, missed that. So, would it not be easier just to add an update_rate > callback to monitor sources (that have a sink with an update_rate > callback?), if that is our indicator that a sink/source supports > updating rates? I don't know if it would be easier. This patch is not complex either. I did briefly consider adding a callback, but I thought that implementing callbacks in the core is sort of ugly, because it adds indirection where indirection isn't needed. That's not a strong opinion, though - I see how it would make pa_source_update_rate() a bit easier to follow. However, now that I try to think how to implement patch 8 in a callback, I don't think it's possible to do cleanly. The passthrough flag is the problem: it's not passed to the callback. For that reason I'd like to keep this patch as it is. -- Tanu
