Dne 15.12.2017 (pet) ob 04:01 +0200 je Tanu Kaskinen napisal(a):
> On Wed, 2017-12-13 at 22:14 +0100, Samo PogaÄ?nik wrote:
> >
> > Thank you for the remarks. I tried to correct all issues and
> > below is a new patch.
> Thanks. I recommend you send patches with "git send-email" in the
> future. That way the formatting will always be right (I had some
> trouble getting git accept the patch), the patch will have a commit
> message and the author and date will be correct.
Sorry for the trouble, i will improve my git skills.
>
> >
> > +static int process_render_use_timing(struct userdata *u, pa_usec_t
> > now) {
> > +Â Â Â Â int ret = 0;
> > +Â Â Â Â size_t dropped = 0;
> > +Â Â Â Â size_t consumed = 0;
> > +
> > +Â Â Â Â pa_assert(u);
> > +
> > +Â Â Â Â /* This is the configured latency. Sink inputs connected to us
> > +Â Â Â Â might not have a single frame more than the maxrequest value
> > +Â Â Â Â queued. Hence: at maximum read this many bytes from the sink
> > +Â Â Â Â inputs. */
> I could have complained about this earlier: this comment is a bit
> hard
> to understand. What does "this" refer to? (Apparently to max_request
> or
> block_usec.) Either remove the comment or rewrite it so that it's
> easy
> to understand.
I though this must be good comment, since it comes from a sample
module. However, i fully accept your remark and i would just remove the
comment for now. Do you agree?Â
>
> >
> > +
> > +Â Â Â Â /* Fill the buffer up the latency size */
> > +Â Â Â Â while (u->timestamp < now + u->block_usec) {
> > +Â Â Â Â Â Â Â Â ssize_t written = 0;
> > +Â Â Â Â Â Â Â Â pa_memchunk chunk;
> > +
> > +Â Â Â Â Â Â Â Â pa_sink_render(u->sink, u->sink->thread_info.max_request,
> > &chunk);
> > +
> > +Â Â Â Â Â Â Â Â pa_assert(chunk.length > 0);
> > +
> > +Â Â Â Â Â Â Â Â if ((written = pipe_sink_write(u, &chunk)) < 0) {
> > +Â Â Â Â Â Â Â Â Â Â Â Â written = -1 - written;
> > +Â Â Â Â Â Â Â Â Â Â Â Â ret = -1;
> > +Â Â Â Â Â Â Â Â }
> Why don't you return immediately here? It doesn't make sense to me to
> continue after writing has failed.
>
> If you return immediately, pipe_sink_write() can then simply return
> -1.
Here i tried to be as accurate as possible regarding the drops counter,
but if we agree, that drop counting becomes irrelevant upon real fifo
error, then i would not bother returning number of potentially written
bytes within pipe_sink_write() before error condition occurred. What do
you say?
Â
>
> >
> > +
> > +Â Â Â Â Â Â Â Â pa_memblock_unref(chunk.memblock);
> > +
> > +Â Â Â Â Â Â Â Â u->timestamp += pa_bytes_to_usec(chunk.length, &u->sink-
> > >sample_spec);
> > +
> > +Â Â Â Â Â Â Â Â dropped = chunk.length - written;
> > +
> > +Â Â Â Â Â Â Â Â if (dropped != 0) {
> > +Â Â Â Â Â Â Â Â Â Â Â Â if (u->bytes_dropped == 0) {
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pa_log_debug("Pipe-sink just dropped %zu bytes",
> > dropped);
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â u->bytes_dropped = dropped;
> > +Â Â Â Â Â Â Â Â Â Â Â Â } else
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â u->bytes_dropped += dropped;
> > +Â Â Â Â Â Â Â Â } else {
> > +Â Â Â Â Â Â Â Â Â Â Â Â if (u->bytes_dropped != 0) {
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pa_log_debug("Pipe-sink continuously dropped %zu
> > bytes", u->bytes_dropped);
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â u->bytes_dropped = 0;
> > +Â Â Â Â Â Â Â Â Â Â Â Â }
> > +Â Â Â Â Â Â Â Â }
> This still doesn't handle the case of two subsequent partial writes
> correctly. The code treats that case as one continuous drop, but
> since
> the second write writes something, it's not a continuous drop.
>
> I think the logic should be something like this:
>
> if (u->bytes_dropped != 0 && dropped != chunk.length) {
> Â Â Â Â pa_log_debug("Pipe-sink continuously dropped %zu bytes", u-
> >bytes_dropped);
> Â Â Â Â u->bytes_dropped = 0;
> }
>
> if (u->bytes_dropped == 0 && dropped != 0)
> Â Â Â Â pa_log_debug("Pipe-sink just dropped %zu bytes", dropped);
>
> u->bytes_dropped += dropped;
>
Silly me. Thank you for the suggestion.
regards, Samo
