1302632 - Incorrect error fired when media segment is added prior an init segment

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

The issue isn't specifically related to MSE, but would be mostly seen there.

When endOfStream() algorithm is called with error set to "decode"

we read:
https://w3c.github.io/media-source/index.html#end-of-stream-algorithm
"
If error is set to "decode"

    If the HTMLMediaElement.readyState attribute equals HAVE_NOTHING
        Run the "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" steps of the resource fetch algorithm's media data processing steps list.
    If the HTMLMediaElement.readyState attribute is greater than HAVE_NOTHING
        Run the media data is corrupted steps of the resource fetch algorithm's media data processing steps list.

"

If a media segment is added before an init segment, we are to call the endOfStream algorithm with the error set to "decode"
As no init segment has been added yet, we are in HTMLMediaElement.readyState == HAVE_NOTHING case

part of the resource fetch algorithm, 
"Failed with media provider: Reaching this step indicates that the media resource failed to load. Queue a task to run the dedicated media source failure steps."

The dedicated media source failure steps being:
https://www.w3.org/TR/html51/semantics-embedded-content.html#dedicated-media-source-failure-steps
"Set the error attribute to a new MediaError object whose code attribute is set to MEDIA_ERR_SRC_NOT_SUPPORTED. "

error.code should be set to MEDIA_ERR_SRC_NOT_SUPPORTED, currently it is set to MEDIA_ERR_DECODE

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1302632: P1. Set proper error code when readyState is HAVE_NOTHING.

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

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1302632: P2. Only have Description return a non-empty string in case of error.

Will simplify the code in the following patch, not having to perform unnecessary test.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1302632: P3. Provide failures details to error attribute.

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

Comment 4

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

Comment on attachment 8791068 [details]
Bug 1302632: P1. Set proper error code when readyState is HAVE_NOTHING.

https://reviewboard.mozilla.org/r/78628/#review77256

::: dom/html/HTMLMediaElement.cpp:4433
(Diff revision 1)
>    ReportLoadError("MediaLoadDecodeError", params, ArrayLength(params));
>  
>    if (mDecoder) {
>      ShutdownDecoder();
>    }
> +  bool isMediaSource = !!mMediaSource;

Why not checking if (mMediaSource ...) as above so we don't need this temp variable?

::: dom/html/HTMLMediaElement.cpp:4447
(Diff revision 1)
>        DispatchAsyncSourceError(mSourceLoadCandidate);
>        QueueLoadFromSourceTask();
>      } else {
>        NS_WARNING("Should know the source we were loading from!");
>      }
>    } else {

Do:

else if (...) {
} else {
}

so we have only 1 level of if/else

Comment 5

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

Comment on attachment 8791069 [details]
Bug 1302632: P2. Only have Description return a non-empty string in case of error.

https://reviewboard.mozilla.org/r/78630/#review77258

Comment 6

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

Comment on attachment 8791070 [details]
Bug 1302632: P3. Provide failures details to error attribute.

https://reviewboard.mozilla.org/r/78632/#review77260

::: dom/html/HTMLMediaElement.cpp:1043
(Diff revision 1)
>  
>    mEventDeliveryPaused = false;
>    mPendingEvents.Clear();
>  }
>  
> -void HTMLMediaElement::NoSupportedMediaSourceError()
> +void HTMLMediaElement::NoSupportedMediaSourceError(const MediaResult& aErrorDetails)

You pass a MediaResult but only use its Description(). Might be better to just pass an nsCString.

Comment 7

[Jean-Yves Avenard [:jya]]

Upon further thought (and more reading).

I think this is not MSE specific anyway.

If we're unable to fetch the content or if the content type can't be determined (e.g. unable to read the metadata), then we should set MediaError.code to MEDIA_ERR_SRC_NOT_SUPPORTED.

That's my reading of the spec.
There's no distinction in the spec between:
"If the media data cannot be fetched at all, due to network errors, causing the user agent to give up trying to fetch the resource" steps and the "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" in the resource fetch algorithm
https://www.w3.org/TR/html51/semantics-embedded-content.html#resource-fetch-algorithm

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791068 [details]
Bug 1302632: P1. Set proper error code when readyState is HAVE_NOTHING.

https://reviewboard.mozilla.org/r/78628/#review77256

> Why not checking if (mMediaSource ...) as above so we don't need this temp variable?

because mMediaSource is set to null in between.

Having said that, setting mMediaSource to null is wrong and will be addressed in bug 1302656

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791070 [details]
Bug 1302632: P3. Provide failures details to error attribute.

https://reviewboard.mozilla.org/r/78632/#review77260

> You pass a MediaResult but only use its Description(). Might be better to just pass an nsCString.

ok

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1302632: P4. Do not change network state to NETWORK_EMPTY.

There's no such step defined in the spec. Despite, this code can no longer be called when readyState is HAVE_NOTHING

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

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791068 [details]
Bug 1302632: P1. Set proper error code when readyState is HAVE_NOTHING.

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

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791069 [details]
Bug 1302632: P2. Only have Description return a non-empty string in case of error.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791070 [details]
Bug 1302632: P3. Provide failures details to error attribute.

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

Comment 14

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

Comment on attachment 8791485 [details]
Bug 1302632: P4. Do not change network state to NETWORK_EMPTY.

https://reviewboard.mozilla.org/r/78924/#review77526

Comment 15

[Jean-Yves Avenard [:jya]]

JW, could you have a look on P1? I've removed the check that it was only applicable to MSE.

Comment 16

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

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Upon further thought (and more reading).
> 
> I think this is not MSE specific anyway.
> 
> If we're unable to fetch the content or if the content type can't be
> determined (e.g. unable to read the metadata), then we should set
> MediaError.code to MEDIA_ERR_SRC_NOT_SUPPORTED.
> 
> That's my reading of the spec.
> There's no distinction in the spec between:
> "If the media data cannot be fetched at all, due to network errors, causing
> the user agent to give up trying to fetch the resource" steps and the "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" in the resource
> fetch algorithm
> https://www.w3.org/TR/html51/semantics-embedded-content.html#resource-fetch-
> algorithm

https://html.spec.whatwg.org/multipage/embedded-content.html#dom-mediaerror-media_err_src_not_supported

I think so.

The precondition of MEDIA_ERR_NETWORK and MEDIA_ERR_DECODE is "after the resource was established to be usable.".

So P1 LGTM.

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1302632: P5. Update mochitests.

The tests expected that the error code would be MEDIA_ERR_DECODE whenever we fail to open a video. However, MEDIA_ERR_DECODE is to be used only when "An error of some description occurred while decoding the media resource, after the resource was established to be usable."

All those files have errors in their metadata. Which makes the resource unusable to start with.

Similarly, networkState would be set to  NETWORK_NO_SOURCE if the metadata couldn't be read.

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

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791068 [details]
Bug 1302632: P1. Set proper error code when readyState is HAVE_NOTHING.

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

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791069 [details]
Bug 1302632: P2. Only have Description return a non-empty string in case of error.

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

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791070 [details]
Bug 1302632: P3. Provide failures details to error attribute.

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

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791485 [details]
Bug 1302632: P4. Do not change network state to NETWORK_EMPTY.

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

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8791832 [details]
Bug 1302632: P5. Update mochitests.

https://reviewboard.mozilla.org/r/79100/#review77710

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d7558864a66
P1. Set proper error code when readyState is HAVE_NOTHING. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/22772e4a9c72
P2. Only have Description return a non-empty string in case of error. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b20f7c887e6c
P3. Provide failures details to error attribute. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/55e7962d4fc0
P4. Do not change network state to NETWORK_EMPTY. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f2ced742d6aa
P5. Update mochitests. r=gerald

Comment 24

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/3172114c06cdf084d9e46c394c47e62c672c7259 because Try says it's not going to go that well after all.

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791068 [details]
Bug 1302632: P1. Set proper error code when readyState is HAVE_NOTHING.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78628/diff/3-4/

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791069 [details]
Bug 1302632: P2. Only have Description return a non-empty string in case of error.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78630/diff/3-4/

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791070 [details]
Bug 1302632: P3. Provide failures details to error attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78632/diff/3-4/

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791485 [details]
Bug 1302632: P4. Do not change network state to NETWORK_EMPTY.

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

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791832 [details]
Bug 1302632: P5. Update mochitests.

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc8cd368bace
P1. Set proper error code when readyState is HAVE_NOTHING. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/e628ce8270d7
P2. Only have Description return a non-empty string in case of error. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/0cb8732f7d66
P3. Provide failures details to error attribute. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/7442beabc153
P4. Do not change network state to NETWORK_EMPTY. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3c4a5256eff7
P5. Update mochitests. r=gerald

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc8cd368bace
https://hg.mozilla.org/mozilla-central/rev/e628ce8270d7
https://hg.mozilla.org/mozilla-central/rev/0cb8732f7d66
https://hg.mozilla.org/mozilla-central/rev/7442beabc153
https://hg.mozilla.org/mozilla-central/rev/3c4a5256eff7