1901483 - Hit MOZ_DIAGNOSTIC_ASSERT(!mOutputTrackSources.Contains(id)) at HTMLMediaElement.cpp:3915

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

Recent new topcrash, see bug 1802394 comment 12.
Tracking this separately from bug 1802394 because it hits a different assertion.

Looking at the code we'd fail this assert if capturing a HTMLMediaElement and adding two MediaTracks with duplicate ids to it. Looking at the URLs this happens with playback, likely MSE. We don't support multiple tracks of any one track kind with any playback case.

That leaves one failure mode: a duplicate MetadataLoaded(). Tracing through what can call that I see MediaSourceDecoder::Load and bug 1899646 which fits the timing of this starting (a May 31 Nightly buildid).

Alastor, can you take a look?

Comment 1

[Alastor Wu [:alwu]]

The bug 1899646 was about reverting a change (D203062) of preventing sending loading metadata twice when switching the normal MDSM and external engine MDSM. So reverting that change just brought us back to what we were before. If this issue happens after bug 1899646, that probably means the crash had been hidden by D203062 and should be solved properly.

In addition, in bug 1899646, I was about adding an implementation to force a media element not to dispatch metadataload multiple times, but I didn't do that because I found this test is intended to have metadataload event multiple times.

Comment 2

[Paul Adenot (:padenot)]

Alastor, are we concerned about this? Can you assign a prio?

Comment 3

[Andreas Pehrson [:pehrsons]]

(In reply to Alastor Wu [:alwu] from comment #1)

The bug 1899646 was about reverting a change (D203062) of preventing sending loading metadata twice when switching the normal MDSM and external engine MDSM. So reverting that change just brought us back to what we were before. If this issue happens after bug 1899646, that probably means the crash had been hidden by D203062 and should be solved properly.

Ok so, in order:

• bug 1884016 prevents more than one loadedmetadata in wmfme
• bug 1881620 enables wmfme in nightly and early beta
• bug 1899646 allows more than one loadedmetadata in wmfme -> this bug

Seems to me the regressor is marked correctly, at least. Good to know it's limited to nightly and early beta. Could you block shipping wmfme any further on this bug, please?

In addition, in bug 1899646, I was about adding an implementation to force a media element not to dispatch metadataload multiple times, but I didn't do that because I found this test is intended to have metadataload event multiple times.

That seems wrong, looking at spec. I imagine this is too, given that loadedmetadata is fired regardless of readyState in HTMLMediaElement::MetadataLoaded().

Comment 4

[Alastor Wu [:alwu]]

(In reply to Andreas Pehrson [:pehrsons] from comment #3)
Okay I can take a look on this again.

Comment 5

[Alastor Wu [:alwu]]

The callstack of crashes are related with audio capturing, but media engine would only be used by EME. I suppose capturing should not happen on EME playback, and we have a check to block capturing audio. That check is checking whether media key exists, but the media key can be attached after capturing. I wonder what would happen in this case? (capturing an element first, then attaching a media key on that element) Would we terminate those streams? or we do other things to block reading audio?

(In reply to Andreas Pehrson [:pehrsons] from comment #3)

That seems wrong, looking at spec. I imagine this is too, given that loadedmetadata is fired regardless of readyState in HTMLMediaElement::MetadataLoaded().

You mean we should only dispatch loadedmetadata once?

Comment 6

[Alastor Wu [:alwu]]

The spec says loadedmetadata should be dispatched readyState is newly equal to HAVE_METADATA or greater for the first time., I am not sure if we should interpret this as loadedmetadata should only dispatched once. Our current implementation doesn't consider ready state when dispatching that event.

However, the spec also defines the event as The user agent has just determined the duration and dimensions of the media resource and the text tracks are ready. From this point of view, it seems to me that dispatching multiple loadedmetadata is reasonable. eg. for DRM clearlead playback, the duration and resolution for clear and encrypted part can be different. Any thought?

Comment 7

[Alastor Wu [:alwu]]

Maybe we should change this to whether the info is encrypted or not. But what will happen if the capturing happens before a media element loads the metadata? Is that allowed?

Comment 8

[Andreas Pehrson [:pehrsons]]

(In reply to Alastor Wu [:alwu] from comment #6)

The spec says loadedmetadata should be dispatched readyState is newly equal to HAVE_METADATA or greater for the first time., I am not sure if we should interpret this as loadedmetadata should only dispatched once.

That text is non-normative. This is the only normative place that fires loadedmetadata, i.e. it only ever happens when moving away from readyState HAVE_NOTHING, which is only ever set in the load algorithm, which I don't think should run more than once in the chained-ogg or EME cases described above.

Our current implementation doesn't consider ready state when dispatching that event.

It relies on the decoder to follow the spec in this regard.

However, the spec also defines the event as The user agent has just determined the duration and dimensions of the media resource and the text tracks are ready. From this point of view, it seems to me that dispatching multiple loadedmetadata is reasonable. eg. for DRM clearlead playback, the duration and resolution for clear and encrypted part can be different. Any thought?

Also non-normative I'm afraid. Note that for duration changes there is the durationchange event, and for dimension changes there's the resize event.

(In reply to Alastor Wu [:alwu] from comment #7)

Maybe we should change this to whether the info is encrypted or not. But what will happen if the capturing happens before a media element loads the metadata? Is that allowed?

It's allowed. I don't recall exactly how we handle this in the decoder, but either the captured stream doesn't see any tracks, or the tracks are contentless or muted, or script is blocked from accessing the track contents through the principal that follows along.

Comment 9

[Donal Meehan [:dmeehan]]

Setting 128 to disabled based on Comment 3 since media.wmf.media-engine.enabled is early beta only

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1901483 - do not report metadataload again when switching from media engine playback to normal playback.

We initially added this in D203062, but remove it in D212064. The reason
of removing it is because not setting up the status for a newly created
MDSM causes Netflix's problem.

However, the situation of reinforming metadata load still exists, eg.
switching from the media engine playback to the normal playback,
which could be seen in the clearlead situation. That seems a possible
case to hit the assertion [1]. Therefore, we should add back the
mechanism of not informing the metadata load again, but also keep the
status being set properly.

In addition, we will handle the process of dispatching loadedmetadata
properly in bug 1906206.

[1]
https://searchfox.org/mozilla-central/rev/a67f92611070fcdba22a106ee140ab0059cc4611/dom/html/HTMLMediaElement.cpp#3916

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed87b01b8b2b
do not report metadataload again when switching from media engine playback to normal playback. r=media-playback-reviewers,padenot

Comment 12

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/ed87b01b8b2b