Closed Bug 1276495 Opened 3 years ago Closed 3 years ago

Crash in mozilla::MediaFormatReader::RequestAudioData

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jya, Assigned: kamidphish)

References

(Blocks 1 open bug)

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

https://crash-stats.mozilla.com/signature/?product=Firefox&release_channel=nightly&platform=Windows&date=%3E%3D2016-05-15&signature=mozilla%3A%3AMediaFormatReader%3A%3ARequestAudioData

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:
https://crash-stats.mozilla.com/report/index/6f92ecbf-64ad-4540-978f-e65ff2160518

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: https://reviewboard.mozilla.org/r/56194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56194/
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

https://reviewboard.mozilla.org/r/56194/#review52824

::: 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

https://reviewboard.mozilla.org/r/56194/#review52826
Attachment #8757773 - Flags: review?(jyavenard) → review+
(In reply to JW Wang [:jwwang] from comment #3)

JW, I created bug 1276570 to implement this.
https://hg.mozilla.org/mozilla-central/rev/066a18a85a04
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.