1130155 - Avoid assert failure in cubeb_alsa.c when consuming only part of buffer

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P2)

Description

[Jan Beich]

This helps FreeBSD with alsa-plugins-oss e.g., when buffer size isn't power of 2.

cf. bug 1021761 comment 45.

Comment 1

[Jan Beich]

Attachment: fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=693d50137f4e

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8560154 [details] [diff] [review]
fix

Review of attachment 8560154 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!  Looks good, just a few minor things to fix.

Please remove #include <stdio.h> and the debugging fprintfs.

::: media/libcubeb/src/cubeb_alsa.c
@@ +327,4 @@
>  
> +    if (avail > 0)
> +      break;
> +    if (pipefailures++ > 11) {

Why 11 here and 3 for snd_pcm_writei?

Also, can you please make these numbers (and the one for EAGAIN) into named constants?

@@ +364,5 @@
>    pthread_mutex_lock(&stm->mutex);
>    if (got < 0) {
>      pthread_mutex_unlock(&stm->mutex);
>      stm->state_callback(stm, stm->user_ptr, CUBEB_STATE_ERROR);
> +    free(p);

Good catch.

@@ +383,5 @@
>        }
>      }
> +    for (;;) {
> +      wrote = WRAP(snd_pcm_writei)(stm->pcm, p,
> +        towrite > avail ? avail : towrite);

Make these two lines into a single line.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Jan - do you want to finish this off?

Comment 4

[Jan Beich]

Before continuing with review it'd be nice to get a feedback from Mikhail (the author) regarding
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=203732

The code is beyond my knowledge as I mostly hack on build glue (here and downstream), so "maybe later" on my end.

Comment 5

[Mikhail T.]

Jan, I'm not sure, what info you are requesting from me... Whether I want to finish the work? I can't -- I don't have the commit-privileges at Mozilla.

The problems identified by Mathew (see Comment #2) are too trivial to warrant another submission (and another review-request), in my opinion -- anyone with modicum of C-language experience can address them to their liking.

Or are you asking about something completely different?

Comment 6

[Jan Beich]

No longer applies after
https://github.com/kinetiknz/cubeb/commit/de49402
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf1de063f92

(In reply to Mikhail T. from comment #5)
> Whether I want to finish the work? 

Whether you want to fix that issue or there's not much point in the patch here.

> I don't have the commit-privileges at Mozilla.

Landing patches only requires review+, a successful Try build and marked with checkin-needed keyword.

> anyone with modicum of C-language experience can address them to their liking.

My C "experience" is limited to ifdefs and some minor sysctl code. For one, in comment 2 Matthew asked about constants and I don't know how they were calculated.

Comment 7

[Matthew Gregan [:kinetik]]

I made a similar set of changes recently to work around a dmix bug, so the current patch will no longer apply.  The snd_pcm_forward call looks wrong from my understanding of the docs.  It's not clear if the retry loops are necessary to me.  It'd be interesting to see what else needs to be changed to get correct playback on FreeBSD.

Comment 8

[Jan Beich]

cubeb_alsa.c no longer crashes on FreeBSD but like with Mikhail's version CPU usage is abnormally high as compared to restricting buffer sizes within alsa-plugins-oss.