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

RESOLVED FIXED

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jbeich, Unassigned)

Tracking

Trunk
All
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox-esr45 affected, firefox50 fixed, firefox51 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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]
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.
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

Updated

3 years ago
Rank: 25
Priority: -- → P2
Jan - do you want to finish this off?
Flags: needinfo?(jbeich)
(Reporter)

Comment 4

3 years ago
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.
Flags: needinfo?(jbeich) → needinfo?(mi+mozilla)

Comment 5

3 years ago
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)
(Reporter)

Comment 6

2 years ago
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.
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.
Depends on: 1247056
(Reporter)

Comment 8

2 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
status-firefox51: --- → fixed
Depends on: 1283850
Resolution: --- → FIXED
(Reporter)

Updated

2 years ago
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → affected
status-firefox48: affected → wontfix
status-firefox49: affected → wontfix
You need to log in before you can comment on or make changes to this bug.