Closed Bug 1130155 Opened 9 years ago Closed 8 years ago

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


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




Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- affected
firefox50 --- fixed
firefox51 --- fixed


(Reporter: jbeich, Unassigned)




(1 file)

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

cf. bug 1021761 comment 45.
Comment on attachment 8560154 [details] [diff] [review]

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.
Attachment #8560154 - Flags: review?(kinetik)
Attachment #8560154 - Flags: review-
Attachment #8560154 - Flags: feedback+
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
Component: Audio/Video: MediaStreamGraph → Audio/Video: cubeb
Rank: 25
Priority: -- → P2
Jan - do you want to finish this off?
Flags: needinfo?(jbeich)
Before continuing with review it'd be nice to get a feedback from Mikhail (the author) regarding

The code is beyond my knowledge as I mostly hack on build glue (here and downstream), so "maybe later" on my end.
Flags: needinfo?(jbeich) → needinfo?(mi+mozilla)
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?
Flags: needinfo?(mi+mozilla)
No longer applies after

(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.
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.
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.
Closed: 8 years ago
Depends on: 1283850
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.