On Thu, 2013-08-22 at 16:47 +0200, David Henningsson wrote:
> On 08/12/2013 02:19 PM, Tanu Kaskinen wrote:
> > On Mon, 2013-08-12 at 13:51 +0200, David Henningsson wrote:
> >> 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.
> >
>
> Ok, maybe this would look a little more elegant than the dual
> comparisons of s->update_rate:
>
> if (s->update_rate) {
> if (!s->update_rate(s, desired_rate))
> return false;
> } else
> s->sample_spec.rate = desired_rate;
If the dual comparisons are the problem, what do you think of the code
in patch 8? The dual comparison is gone, and the code structure is
pretty close to what you propose here.
> (We don't need the monitor_of assertion, we checked that earlier in the
> function.)
The assertion is there to help with reading the code more than to catch
errors. The assertions says that if s->update_rate is not set, then the
source is a monitor source, which might not be obvious to the reader
without the assertion. I can remove the assertion if you don't like it.
--
Tanu
