1272182 - loadedmetadata shouldn't be fired if we have no way of decoding the data

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Bug 1207198 changed the behaviour to accept media's metadata so long as we have a duration and dimensions.

This was changed as the loadedmetadata definition in the spec is:
"The user agent has just determined the duration and dimensions of the media resource and the text tracks are ready. "

however, in the list of actions to be performed prior changing the readyState and firing the loadedmetadata event, we can read:
https://w3c.github.io/html/semantics-embedded-content.html#media-data-processing-steps-list
"If the media data can be fetched but is found by inspection to be in an unsupported format, or can otherwise not be rendered at all "

So, we should check if we can actually decode the video tracks prior accepting the metadata.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272182: P1. Ignore invalid video tracks. r?cpearce

Review commit: https://reviewboard.mozilla.org/r/52091/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52091/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272182: P2. Reject upfront videos for which we have no decoder. r?cpearce

Similar logic with audio tracks, but those are ignored instead.

Review commit: https://reviewboard.mozilla.org/r/52093/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52093/

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8751557 [details]
MozReview Request: Bug 1272182: P1. Ignore invalid video tracks. r?cpearce

https://reviewboard.mozilla.org/r/52091/#review49015

::: dom/media/MediaFormatReader.cpp:301
(Diff revision 1)
> -    }
> +      }
> -    mVideo.mCallback = new DecoderCallback(this, TrackInfo::kVideoTrack);
> +      mVideo.mCallback = new DecoderCallback(this, TrackInfo::kVideoTrack);
> -    mVideo.mTimeRanges = mVideo.mTrackDemuxer->GetBuffered();
> +      mVideo.mTimeRanges = mVideo.mTrackDemuxer->GetBuffered();
> -    mTrackDemuxersMayBlock |= mVideo.mTrackDemuxer->GetSamplesMayBlock();
> +      mTrackDemuxersMayBlock |= mVideo.mTrackDemuxer->GetSamplesMayBlock();
> +    } else {
> +      mVideo.mTrackDemuxer->BreakCycles();

Other places that call m{Audio,Video}.mTrackDemuxer->BreakCycles() also call m{Audio,Video}.mTrackDemuxer m{Audio,Video}.ResetDemuxer(). Should you also be doing that here?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8751558 [details]
MozReview Request: Bug 1272182: P2. Reject upfront videos for which we have no decoder. r?cpearce

https://reviewboard.mozilla.org/r/52093/#review49017

::: dom/media/MediaFormatReader.cpp:306
(Diff revision 1)
> +      mMetadataPromise.Reject(ReadMetadataFailureReason::METADATA_ERROR, __func__);
> +      return;
> +    }
> +
>      if (videoActive) {
>        mInfo.mVideo = *videoInfo->GetAsVideoInfo();

You could nest the "platform && !platform->SupportsMimeType" check inside this videoActive check, to save checking it twice.

::: dom/media/MediaFormatReader.cpp:329
(Diff revision 1)
>        return;
>      }
> +
>      UniquePtr<TrackInfo> audioInfo = mAudio.mTrackDemuxer->GetInfo();
>      // We actively ignore audio tracks that we know we can't play.
>      audioActive = audioInfo && audioInfo->IsValid();

You could calculate the value of audioActive in one statement instead of two.

Comment 5

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/52091/#review49015

> Other places that call m{Audio,Video}.mTrackDemuxer->BreakCycles() also call m{Audio,Video}.mTrackDemuxer m{Audio,Video}.ResetDemuxer(). Should you also be doing that here?

it will be done when the MFR will shutdown.

The only reason I had to shut down the track demuxer, is that HasAudio() and HasVideo() are actually return !!mAudio/mVideo.mTrackDemuxer

Comment 6

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/52091/#review49015

> it will be done when the MFR will shutdown.
> 
> The only reason I had to shut down the track demuxer, is that HasAudio() and HasVideo() are actually return !!mAudio/mVideo.mTrackDemuxer

oh, and also, the trackdemuxer haven't been used yet. so no need to reset them

Comment 7

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/52093/#review49017

> You could calculate the value of audioActive in one statement instead of two.

yeah, but then it makes it all go unto multiple line with extra parantheses; I figured it was clearer to read that way

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8354c6f439c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5eac62c7880

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out as requested by jya for missing the latest updates:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9a2d8eddfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2ceebf75b7

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751558 [details]
MozReview Request: Bug 1272182: P2. Reject upfront videos for which we have no decoder. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52093/diff/1-2/

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9673bccca574
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c56040ec81

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9673bccca574
https://hg.mozilla.org/mozilla-central/rev/45c56040ec81