Closed Bug 1008079 Opened 6 years ago Closed 6 years ago

Assertion failure: !mAudioStream->IsPaused() (Don't play when paused), at /builds/slave/try-and-d-00000000000000000000/build/content/media/MediaDecoderStateMachine.cpp:992

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files, 3 obsolete files)

Blocks: 1006246
See https://bugzilla.mozilla.org/show_bug.cgi?id=1006246#c5.

Sample rate = 11127000 will cause AudioStream::Init to fail on Android and therefore MediaDecoderStateMachine can't play normally for MediaDecoderStateMachine::GetAudioClock() can return incorrect results.

Should we
1. remove files with strange sample rates from our media test or
2. make MediaDecoderStateMachine handle AudioStream::Init failures?

I think it would be weird to the user to play the file normally without sound output. I would prefer it just bails out by saying "The format not supported".
Flags: needinfo?(rjesup)
Redirecting to roc
Flags: needinfo?(rjesup) → needinfo?(roc)
Duplicate of this bug: 1006262
On the other hand, the media element can be captured for streaming, we should be able to play even if we fail to init AudioStream as we did in WebAudio.
We should resample those files to a rate Android can play.
Flags: needinfo?(roc)
(In reply to JW Wang [:jwwang] from comment #4)
> On the other hand, the media element can be captured for streaming, we
> should be able to play even if we fail to init AudioStream as we did in
> WebAudio.

JW, which file is that? A number like that looks like a bug to me, a sound card that can support a sample rate like that simply does not exist (eleven million hertz!), and resampling that will cost us a fortune in CPU time. I would not be surprised of a race or other data corruption because of concurrency when cloning the decoder and friends (which is what the test is doing, if I read correctly).
Flags: needinfo?(jwwang)
Sorry for causing the confusion. The sample rate reported by cubeb_opensl.c is in milliHertz.
See http://dxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb_opensl.c#406.
Flags: needinfo?(jwwang)
Okay, resample away then.

A way to do this would be to have an API in cubeb that tells you the sample rate supported, and then check if you have to resample, and then insert a resampler in the pipeline.

Another way to do this would be to have cubeb resample automagically and support all samples rates, but only on the backends that don't do this themselves (this is what we do in the WASAPI backend, I'm sure code can be borrowed, it's the exact same problem). This is likely to be faster, it's just slightly more complicated than a copy and paste (because we would split the resampling logic into its own file).
I choose option 1 to insert a resampler in MediaDecoderStateMachine.

For option 2, my concern is MediaDecoderStateMachine could write silent frames to AudioStream. Sharp changes in audio values could produce high frequency noise after resampling. But I haven't had a test to prove how it would be.
Our resampler is very good, this should not be a concern. fwiw this is exactly what every other backends do so that they can play audio at arbitrary sample rate. I think we should do option 2.
Ok, lets go for option 2. Can you show me the resampling in WASAPI for reference?
[1] is the actual resampling.

The resampling path is set up in wasapi_stream_init, you can see how we hook up the right method, depending on if we also need to {up,down}mix (which we don't have to do, here). 

[1]: http://dxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb_wasapi.cpp?from=media/libcubeb/src/cubeb_wasapi.cpp#216
It looks like adding a resampler in cubeb.c will break the concise and simple/thin layer of cubeb.c. Is it better to insert the resampler into cubeb_opensl.c for AudioStream::Init failure only happens on Android?
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8426046 - Flags: feedback?(paul)
I think so, yes. Most of the platform resample automatically, we just need to do it for the platform where the sample rate is out of range, or if the platform does not support resampling. Again, it might be worthwhile to share the code between the two platforms (wasapi/windows and opensles/android).
Yeah, I was also thinking about extracting the resampling code in wasapi so it is resuable in opensles.
Attachment #8426046 - Attachment is obsolete: true
Attachment #8426046 - Flags: feedback?(paul)
Extract the resampling code from cubeb_wasapi.cpp so it is reusable.
Attachment #8428491 - Flags: feedback?(rlin)
Attachment #8428491 - Flags: feedback?(ayang)
Hi Paul,

http://dxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb_opensl.c#604

The AudioFlinger latency part is suspicious to me. Shouldn't it return frames in input rate instead of the preferred rate for cubeb_stream_get_latency returns latency in frames in input rate (which is stream_params.rate passed to cubeb_stream_init)?
Flags: needinfo?(paul)
(In reply to JW Wang [:jwwang] from comment #17)
> Hi Paul,
> 
> http://dxr.mozilla.org/mozilla-central/source/media/libcubeb/src/
> cubeb_opensl.c#604
> 
> The AudioFlinger latency part is suspicious to me. Shouldn't it return
> frames in input rate instead of the preferred rate for
> cubeb_stream_get_latency returns latency in frames in input rate (which is
> stream_params.rate passed to cubeb_stream_init)?

Indeed you're right.
Flags: needinfo?(paul)
(In reply to JW Wang [:jwwang] from comment #16)
> Created attachment 8428491 [details] [diff] [review]
> 1008079_cubeb_resampling_part1-v1.patch
> 
> Extract the resampling code from cubeb_wasapi.cpp so it is reusable.

try is green: https://tbpl.mozilla.org/?tree=Try&rev=4cb282bb4fe1
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +1,2 @@
> +/*
> + * Copyright � 2014 Mozilla Foundation

unknown character

@@ +85,5 @@
> +
> +  virtual long Fill(void* buffer, long framesNeeded);
> +
> +private:
> +

nits: remove this line.

::: media/libcubeb/src/cubeb_resampler.h
@@ +1,2 @@
> +/*
> + * Copyright � 2014 Mozilla Foundation

^^^

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +1,2 @@
>  /*
> + * Copyright � 2013 Mozilla Foundation

-----------------^^^^

@@ +205,5 @@
>      dest = data;
>    }
>  
> +  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
> +

How about the CUBEB_ERROR return code?

@@ +206,5 @@
>    }
>  
> +  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
> +
> +  /* XXX: Handle this error. */

What's XXX means?
Attachment #8428491 - Flags: feedback?(rlin) → feedback+
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +205,5 @@
>      dest = data;
>    }
>  
> +  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
> +

See http://dxr.mozilla.org/mozilla-central/source/media/libcubeb/include/cubeb.h#156.
The signature conforms to that of cubeb_data_callback where return value < 0 indicates an error.

@@ +206,5 @@
>    }
>  
> +  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
> +
> +  /* XXX: Handle this error. */

XXX is used all around in libcubeb which should mean "TODO", I think.
Attachment #8428491 - Flags: review?(paul)
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +71,5 @@
> +private:
> +  cubeb_stream* const mStream;
> +  const cubeb_data_callback mDataCallback;
> +  void* const mUserPtr;
> +};

No virtual destructor? Since you use base class 'cubeb_resampler' point to this class, I think you need to provide a destructor here.
Attachment #8428491 - Flags: feedback?(ayang)
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +71,5 @@
> +private:
> +  cubeb_stream* const mStream;
> +  const cubeb_data_callback mDataCallback;
> +  void* const mUserPtr;
> +};

Since cubeb_resampler already has a virtual destructor, NullResampler doesn't need to define its own if it has nothing to do in its destructor.
Blocks: 903525
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

We use snake_case instead of CamelCase in cubeb.

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +49,5 @@
> +  virtual long Fill(void* buffer, long framesNeeded) = 0;
> +  virtual ~cubeb_resampler() {}
> +};
> +
> +class NullResampler : public cubeb_resampler {

This should be called "passthrough" or "no-op" or something, because it does not actually null-out the samples, it just does not touch them.

@@ +95,5 @@
> +
> +  // Maximum number of frames we can be requested in a callback.
> +  const long mBufferFrameCount;
> +  // input rate / output rate
> +  const float mSamplingRatio;

Call this resampling ratio

@@ +104,5 @@
> +
> +  // A little buffer to store the leftover frames,
> +  // that is, the samples not consumed by the resampler that we will end up
> +  // using next time Fill() is called.
> +  uint8_t* mLeftoverFramesBuffer;

space before and after the *, please.

@@ +123,5 @@
> +  , mSamplingRatio(static_cast<float>(params.rate) / outRate)
> +  , mLeftoverFrameSize(static_cast<uint32_t>(ceilf(1 / mSamplingRatio * 2) + 1))
> +  , mLeftoverFrameCount(0)
> +{
> +  mLeftoverFramesBuffer = new uint8_t[frames_to_bytes(params, mLeftoverFrameSize)];

Since this is C++, now, can you put that in a little RAII class? Something like:

template<typename T>
class AutoArray {
public:
   AutoArray(uint32_t elem_count) { array = new T[elem_count]; }
  ~AutoArray() { delete array; }
private:
   T* array;
};

We could even store the size inside the array, and have bound checking in debug mode, this is easy enough to implement. Your call, you can also open a followup bug so someone can implement it.

@@ +134,5 @@
> +}
> +
> +Resampler::~Resampler()
> +{
> +  speex_resampler_destroy(mResampler);

We should wrap that into an RAII class as well.

@@ +170,5 @@
> +    float* inBuffer = reinterpret_cast<float*>(mResamplingSrcBuffer);
> +    float* outBuffer = reinterpret_cast<float*>(buffer);
> +    speex_resampler_process_interleaved_float(mResampler, inBuffer, &inFrames,
> +                                              outBuffer, &outFrames);
> +

nit: blank line

::: media/libcubeb/src/cubeb_resampler.h
@@ +22,5 @@
> +} cubeb_resampler_quality;
> +
> +/**
> + * Create a resampler to adapt the requested sample rate into that accepted
> + * by the audio backend.

"into something that is accepted by the audio backend"

@@ +44,5 @@
> +                                         cubeb_resampler_quality quality);
> +
> +/**
> + * Fill the buffer with frames returned by the data callback. Resampling will
> + * happen if necessary.

"with frames acquired using the data callback"

@@ +47,5 @@
> + * Fill the buffer with frames returned by the data callback. Resampling will
> + * happen if necessary.
> + * @param resampler A cubeb_resampler instance.
> + * @param buffer The buffer to be filled.
> + * @param frames_needed Number of frames that should be filled.

s/filled/produced/

@@ +48,5 @@
> + * happen if necessary.
> + * @param resampler A cubeb_resampler instance.
> + * @param buffer The buffer to be filled.
> + * @param frames_needed Number of frames that should be filled.
> + * @retval Number of frames that are actually filled.

"that were actually produced"

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +206,5 @@
>    }
>  
> +  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
> +
> +  /* XXX: Handle this error. */

It does.

@@ +211,5 @@
> +  if (out_frames < 0) {
> +    assert(false);
> +  }
> +
> +  /* Go to draining mode if we got fewer frames than requested. */

s/to/in/

@@ +301,5 @@
>  
>        BYTE* data;
>        hr = stm->render_client->GetBuffer(available, &data);
>        if (SUCCEEDED(hr)) {
> +        refill_with_resampling(stm, reinterpret_cast<float *>(data), available);

This should have a new name, because we don't resample all the time anymore.
Attachment #8428491 - Flags: review?(paul)
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +206,5 @@
>    }
>  
> +  long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
> +
> +  /* XXX: Handle this error. */

Do you mean the error is already handled so the comment is not needed?

@@ +301,5 @@
>  
>        BYTE* data;
>        hr = stm->render_client->GetBuffer(available, &data);
>        if (SUCCEEDED(hr)) {
> +        refill_with_resampling(stm, reinterpret_cast<float *>(data), available);

I will call it "refill" then.
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +123,5 @@
> +  , mSamplingRatio(static_cast<float>(params.rate) / outRate)
> +  , mLeftoverFrameSize(static_cast<uint32_t>(ceilf(1 / mSamplingRatio * 2) + 1))
> +  , mLeftoverFrameCount(0)
> +{
> +  mLeftoverFramesBuffer = new uint8_t[frames_to_bytes(params, mLeftoverFrameSize)];

Should we still use snake case for the class name (ie: auto_array) when it comes to template?
Comment on attachment 8428491 [details] [diff] [review]
1008079_cubeb_resampling_part1-v1.patch

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

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +123,5 @@
> +  , mSamplingRatio(static_cast<float>(params.rate) / outRate)
> +  , mLeftoverFrameSize(static_cast<uint32_t>(ceilf(1 / mSamplingRatio * 2) + 1))
> +  , mLeftoverFrameCount(0)
> +{
> +  mLeftoverFramesBuffer = new uint8_t[frames_to_bytes(params, mLeftoverFrameSize)];

To be prudent, I will file another bug for such code enhancement after this bug is stablized.
update coding style and comments.
Attachment #8428491 - Attachment is obsolete: true
Attachment #8434732 - Flags: review?(paul)
Comment on attachment 8434732 [details] [diff] [review]
1008079_cubeb_resampling_part1-v2.patch

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

Sorry for the delay with the review, JW.

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +73,5 @@
> +  const cubeb_data_callback data_callback;
> +  void * const user_ptr;
> +};
> +
> +class cubeb_resampler_impl : public cubeb_resampler {

Name this "cubeb_resampler_speex" so it's clear what it's using.

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +111,5 @@
>    HANDLE thread;
>    uint64_t clock_freq;
>    /* Maximum number of frames we can be requested in a callback. */
>    uint32_t buffer_frame_count;
> +  /* Resampler instance. Resampling will happen if necessary. */

s/will happen/will only happen/
Attachment #8434732 - Flags: review?(paul) → review+
Fix nits suggested by Paul.
Attachment #8434732 - Attachment is obsolete: true
Attachment #8441101 - Flags: review+
Use a resampler when the sampling rate not supported by the openSL player.
Attachment #8441105 - Flags: review?(paul)
build system bits.
Attachment #8441106 - Flags: review?(mh+mozilla)
Attachment #8441106 - Flags: review?(mh+mozilla) → review+
Attachment #8441105 - Flags: review?(paul) → review+
Blocks: 1026854
Is there any plan for upstreaming this?  It would've been nice if someone had asked me to review these libcubeb changes.
Sure. What is the procedure to upstream the changes? Sending a pull request to https://github.com/kinetiknz/cubeb? Do I need to also update README_MOZILLA manually?
You need to log in before you can comment on or make changes to this bug.