1276495 - Crash in mozilla::MediaFormatReader::RequestAudioData

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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

Comment 1

[Jean-Yves Avenard [:jya]]

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.

Comment 2

[u480271]

Attachment: MozReview Request: Bug 1276495: Don't reset audio promises for video only seek. r?jwwang,jya

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/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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.

Ｉwould 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 4

[Jean-Yves Avenard [:jya]]

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

Comment 5

[u480271]

(In reply to JW Wang [:jwwang] from comment #3)

JW, I created bug 1276570 to implement this.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/066a18a85a04

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/066a18a85a04