Closed Bug 1276495 Opened 7 years ago Closed 7 years ago

Crash in mozilla::MediaFormatReader::RequestAudioData


(Core :: Audio/Video: Playback, defect, P1)




Tracking Status
firefox49 --- fixed


(Reporter: jya, Assigned: u480271)


(Blocks 1 open bug)


(Keywords: crash, topcrash)

Crash Data


(1 file)

The assertion used to be due to MOZ_DIAGNOSTIC_ASSERT(mSeekPromise.IsEmpty());

Following bug 1273018, the order of the checks was changed and we are now hitting the assertion:
MOZ_DIAGNOSTIC_ASSERT(!mAudio.HasPromise(), "No duplicate sample requests");

But I believe the issue has been there since bug 1224973, just happened to keep hitting another assertions each time, progression down the list
Here is an example I believe proving my theory above:

Only change to the MediaFormatReader here was the change of order in the assertion, before all the work to fix the logic of skip to next video key frame logic.
Priority: -- → P1
Crash Signature: [@ mozilla::MediaFormatReader::RequestAudioData]
Keywords: crash, topcrash
Assignee: nobody → dglastonbury
Blocks: 1276556
ResetDecode was disconnecting mAudioDataRequest when seeking video
only. This means that, if a RequestAudioData() was outstanding,
mAudioDataRequest and MFR.mAudio.mHasPromise would become out-of-sync.

Review commit:
See other reviews:
Attachment #8757773 - Flags: review?(jyavenard)
Attachment #8757773 - Flags: review?(jwwang)
Attachment #8757773 - Flags: review?(jwwang) → review+
Comment on attachment 8757773 [details]
MozReview Request: Bug 1276495: Don't reset audio promises for video only seek. r?jwwang,jya

::: dom/media/MediaDecoderReaderWrapper.cpp:400
(Diff revision 1)
>  void
>  MediaDecoderReaderWrapper::ResetDecode(TargetQueues aQueues)
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +  if (aQueues == MediaDecoderReader::AUDIO_VIDEO) {

MediaDecoderReader::TargetQueues is confusing to me. It is hard to comprehend all fields of MediaDecoderReader::TargetQueues include video when looking at AUDIO_VIDEO alone.

Iwould prefer bit flags like:
enum TargetQueues {
  VIDEO = 0x1,
  AUDIO = 0x2

and code like:
if (flag & MediaDecoderReader::VIDEO) {
  // do something about video
if (flag & MediaDecoderReader::AUDIO) {
  // do something about audio
Comment on attachment 8757773 [details]
MozReview Request: Bug 1276495: Don't reset audio promises for video only seek. r?jwwang,jya
Attachment #8757773 - Flags: review?(jyavenard) → review+
(In reply to JW Wang [:jwwang] from comment #3)

JW, I created bug 1276570 to implement this.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.