Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range verification should not fail media responses #7782

Merged
merged 8 commits into from
May 23, 2022

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Apr 4, 2022

Some User-agents don't actually perform these verifications.
The only mandatory verification is origin of responses.

See #7655 (comment)

(See WHATWG Working Mode: Changes for more details.)


/infrastructure.html ( diff )
/media.html ( diff )

@noamr noamr changed the title Don't verify range values in range responses Don't verify range values in range responses and allow 200 Apr 4, 2022
@noamr noamr mentioned this pull request Apr 4, 2022
3 tasks
@noamr noamr changed the title Don't verify range values in range responses and allow 200 Don't verify Content-Range values in range responses and allow 200 Apr 5, 2022
@ntrrgc
Copy link

ntrrgc commented Apr 8, 2022

At least two implementers are interested (and none opposed):

Source for WebKit not opposed?

Last response from Jer Noble (developer of the multimedia stack on WebKit's Apple ports) in the bug you linked (https://bugs.webkit.org/show_bug.cgi?id=211323) clearly says:

Safari's media frameworks will do not support servers which themselves do not support range requests.

Alicia here, maintainer of the GStreamer ports (WebKitGTK and WPE): I still sustain servers without range request support are ill-suited for media serving. The reasons for that are:

  1. In media with fast-start, seeks are impossible without buffering the whole timeline until the seek time. This is a severe issue for any video that is longer than a couple minutes.
  2. In media without fast-start, performance is even worse as not even the poster can be shown until the whole media file has been completely downloaded locally. On the other hand, media without fast-start is handled smoothly by user agents, with just a small start latency penalty (first request notices the resource is not fast-start, second request downloads the metadata, third request downloads the media).
  3. Range request support is necessary for the simplest and efficient method of saving bandwidth by only downloading the first few seconds of video until the user confirms playback. Either that is impossible (media without fast-start) or it's possible but requires downloading the same portion of the video twice.
  4. Network interruptions or network switching require completely re-downloading the video.
  5. User agents are left to handle where to store the video, which is a problem for constrained devices, as videos often are around 1GB in size. Do you put it in memory? Do you put it in disk? What do you do if the memory is nearly full (not uncommon)? What do you do if the disk is full (also not uncommon) or read-only (not uncommon for embedded devices)?
  6. Detecting lack of support for range requests and implementing all the necessary work-arounds adds significant complexity to the user agent multimedia stack.
  7. Increasing support for servers without range-requests makes developers and system administrators less aware of all the above challenges and less prone to fix them by implementing Range-Request, as playback "seems to work fine".

For all the previous reasons, media playback without Range-Request support in the server -- even with the most fully featured workarounds -- is not possible without significant performance degradation for the user, wasted data usage for the user, the server and the network. It adds significant complexity to the user agents, while encourages developers and server maintainers to forget best practices.

Last thing I want is to encourage CDNs and server maintainers to pay even less attention to Range-Request support.

I oppose.

@noamr
Copy link
Contributor Author

noamr commented Apr 8, 2022

At least two implementers are interested (and none opposed):

Source for WebKit not opposed?

Last response from Jer Noble (developer of the multimedia stack on WebKit's Apple ports) in the bug you linked (https://bugs.webkit.org/show_bug.cgi?id=211323) clearly says:

Safari's media frameworks will do not support servers which themselves do not support range requests.

Apologies, I totally missed that comment. Not sure whether this is meant as an opposition or a limitation but I will let Apple speak for that.

Alicia here, maintainer of the GStreamer ports (WebKitGTK and WPE): I still sustain servers without range request support are ill-suited for media serving. The reasons for that are:

  1. In media with fast-start, seeks are impossible without buffering the whole timeline until the seek time. This is a severe issue for any video that is longer than a couple minutes.

  2. In media without fast-start, performance is even worse as not even the poster can be shown until the whole media file has been completely downloaded locally. On the other hand, media without fast-start is handled smoothly by user agents, with just a small start latency penalty (first request notices the resource is not fast-start, second request downloads the metadata, third request downloads the media).

  3. Range request support is necessary for the simplest and efficient method of saving bandwidth by only downloading the first few seconds of video until the user confirms playback. Either that is impossible (media without fast-start) or it's possible but requires downloading the same portion of the video twice.

  4. Network interruptions or network switching require completely re-downloading the video.

  5. User agents are left to handle where to store the video, which is a problem for constrained devices, as videos often are around 1GB in size. Do you put it in memory? Do you put it in disk? What do you do if the memory is nearly full (not uncommon)? What do you do if the disk is full (also not uncommon) or read-only (not uncommon for embedded devices)?

  6. Detecting lack of support for range requests and implementing all the necessary work-arounds adds significant complexity to the user agent multimedia stack.

  7. Increasing support for servers without range-requests makes developers and system administrators less aware of all the above challenges and less prone to fix them by implementing Range-Request, as playback "seems to work fine".

For all the previous reasons, media playback without Range-Request support in the server -- even with the most fully featured workarounds -- is not possible without significant performance degradation for the user, wasted data usage for the user, the server and the network. It adds significant complexity to the user agents, while encourages developers and server maintainers to forget best practices.

Last thing I want is to encourage CDNs and server maintainers to pay even less attention to Range-Request support.

I oppose.

Thanks for this view, Alicia.

All of the comments above are about servers with no range support at all, which is part of the PR. The other part is about not requiring that the range I. the request and response match, which is not an http requirement and I am not sure if the Apple comment is relevant to that. Could you share your view on that? I am not sure if the Apple or GStreamer ports currently perform this verification.

@noamr
Copy link
Contributor Author

noamr commented Apr 8, 2022

As a way to reflect current state of implementations, I suggest to put the 200/non-matching-range as a MAY and make the WPTs tentative.

I wish though that this can work towards a consensus, the media part of HTML is quite tentative as it is.

@noamr
Copy link
Contributor Author

noamr commented Apr 11, 2022

I've updated the PR to reflect the status quo:

  • mixed-origins with opaque responses are never allowed
  • range unsupported / range mismatch are optionally allowed (Currently Gecko/Chromium allows, WebKit does not)

@ntrrgc, @karlt: thoughts?

@noamr noamr changed the title Don't verify Content-Range values in range responses and allow 200 Make range verification optional Apr 11, 2022
@domenic
Copy link
Member

domenic commented Apr 11, 2022

We don't like optional features in our specifications. Let's align with 2/3 browsers.

@noamr noamr changed the title Make range verification optional Range verification should not fail media responses Apr 11, 2022
@noamr
Copy link
Contributor Author

noamr commented Apr 11, 2022

We don't like optional features in our specifications. Let's align with 2/3 browsers.

Done.

@jernoble
Copy link

I am opposed. Requesting a specific Range and receiving an entirely different Content-Range in response should be treated as an error. UAs can decide to be lenient in the face of a buggy or misconfigured server, but we should not enshrine that behavior as supported in a specification.

@domenic
Copy link
Member

domenic commented Apr 11, 2022

Understood. We can have a tracking issue open for seeing if other browsers are willing to align with Safari's behavior. For now the most important priority is to get the spec to reflect reality.

@karlt
Copy link

karlt commented Apr 11, 2022

In this case, we can't have the spec both avoid optional behavior and reflect reality.

My original question around range requests came from a view that, while consistent with historic WebKit behavior, was apparently not what was intended.

Although "one or more parts" might imply that the server MAY partially respond to the range request, this other sentence might imply that providing only part of what is requested should be limited to situations where the other parts are not available due to the "current length of the representation":

However, a server MAY generate a multipart/byteranges payload with only a single body part if multiple ranges were requested and only one range was found to be satisfiable or only one range remained after coalescing..

Questions around the server providing only part of the requested range were raised on ietf-http-wg and the responses indicate that this was not intended, not "designed for", and perhaps would have been considered poor design.

So the remaining part of my range request question then is whether there is need for the inconsistency of the case where byteRange[1] is "until end"?
If a complete-length is provided in the byte-range-resp, then a consistent verification would check that end match the complete-length.

For byte ranges, a sender SHOULD indicate the complete length of the representation from which the range has been extracted, unless the complete length is unknown or difficult to determine. An asterisk character ("*") in place of the complete-length indicates that the representation length was unknown when the header field was generated.

Plausibly the complete-length may not be known at the time of generating the header field if the Range request has a last-byte-pos less than the "current length of the representation". However the Content-Range header in the response to a "Range: bytes=0-" request will include the last-byte-pos corresponding to the last byte in the representation, and so complete-length will be known.

@jakearchibald wrote the "Validate a partial response" algorithm for background-fetch and so may be interested in this discussion.

@karlt
Copy link

karlt commented Apr 11, 2022

A 200 (OK) response is clearly a valid response, and reasonable at least for a smaller media resource. Is the acceptance of that less contentious?

@jernoble
Copy link

A 200 (OK) response is clearly a valid response,

From the POV of HTTP, it's valid. For all the reasons @ntrrgc mentions above, a server responding to a Range request with a 200 response breaks a number of important media use cases. I would like to avoid the specification implying that a UA which requires a 206 response to a Range request for media is somehow "out of compliance".

...and reasonable at least for a smaller media resource.

This is verging on "how many hairs make a beard" territory. How small a resource is small enough to be reasonable?

@noamr
Copy link
Contributor Author

noamr commented Apr 12, 2022

@jakearchibald wrote the "Validate a partial response" algorithm for background-fetch and so may be interested in this discussion.

That was the basis for the current algorithm, though it's a bit nuanced because of the multiple-origin verification.

@noamr
Copy link
Contributor Author

noamr commented Apr 12, 2022

So the remaining part of my range request question then is whether there is need for the inconsistency of the case where byteRange[1] is "until end"? If a complete-length is provided in the byte-range-resp, then a consistent verification would check that end match the complete-length.

I believe WebKit does a sensible thing by sending a 0-1 range first to receive the first byte together with the total size.
But I see how allowing the response range to be a subset of the requested one should not be too problematic.

Regarding 200, based on the HTTP archive this is quite rare, about 1% of all video requests with a range header receive a 200 response.

@karlt
Copy link

karlt commented Apr 12, 2022

But I see how allowing the response range to be a subset of the requested one should not be too problematic.

Despite the questionable validity of incomplete Partial Content responses, Gecko implemented support for requesting the missing portion after a few compatibility reports.

If incomplete responses to Range: bytes=<N1>- requests are accepted, then, for consistency, incomplete responses to Range: bytes=<N1>-<N2> requests would also be accepted.

I can imagine that having the trailing portion of the range missing might be easier to support than having the leading portion of the range missing, because the leading portion is usually needed first and the trailing portion may also be unavailable for other reasons such as network timeouts. FWIW Gecko seems to re-request if either portion is missing.

@noamr
Copy link
Contributor Author

noamr commented Apr 19, 2022

So to summarize:

  • The current PR ignores what webkit does, which is to not allow 200 respones
  • According to HTTP archive, this mismatch would impact less than 1% of media resources on the web.
  • The HTML standard already allows the UA to fail some media responses

I believe that the right way forward is to put wording that discourages 200 responses and range mismatch, and that though they are permitted, some UAs can fail them for performance reasons (e.g. if downloading the whole response and buffering it on the client side is too costly network-wise)

@domenic?

@domenic
Copy link
Member

domenic commented Apr 21, 2022

I don't think we need to add anything to the spec to allow WebKit's behavior. The spec does not mandate that all network requests succeed---it cannot, because sometimes networks go down or have errors. If 1% of requests happen to fail in WebKit and not in other browsers, that seems very reasonable.

As for whether the spec should discourage 200s, I'm not sure. That doesn't feel like something for the HTML spec to adjudicate; it feels like it belongs in the HTTP specs for Range requests instead?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Apr 22, 2022

I don't think we need to add anything to the spec to allow WebKit's behavior. The spec does not mandate that all network requests succeed---it cannot, because sometimes networks go down or have errors. If 1% of requests happen to fail in WebKit and not in other browsers, that seems very reasonable.

As for whether the spec should discourage 200s, I'm not sure. That doesn't feel like something for the HTML spec to adjudicate; it feels like it belongs in the HTTP specs for Range requests instead?

I think it should discourage those responses, not because of webkit behavior, but because it's a rare anti-pattern that webkit chose to not support.

Discouraging range mismatches belongs in the HTML spec because the problematic implications are on how user-agents handle media resources, which is an application-level concern rather than a protocol problem. In other implementations/uses of range headers this might be totally OK, e.g. a media application like VLC can choose to download the whole resource and perform buffering on the client side if the server doesn't support range requests.

@noamr
Copy link
Contributor Author

noamr commented May 4, 2022

@domenic: the current revision of this PR allows range mismatches, while discouraging them for the aforementioned reasons. How do we proceed?

@domenic
Copy link
Member

domenic commented May 4, 2022

I still don't feel comfortable discouraging 200 responses to range requests in HTML, when it isn't discouraged for HTTP in general. Maybe that part of the PR could be separated out and subject to a larger discussion with the HTTP + browser media community.

noamr added 3 commits May 4, 2022 18:24
Verifying media responses performs 3 verifications:
* The response should also be a range response
* The response's range should match the request's range
* There should not be mixed origins with opaque response

While the last verification is mandatory for same-origin policy, the first two
are optional, and may be skipped if the user-agent decides to be lenient towards
servers who don't handle range requests properly.

See whatwg#7655 (comment)
@noamr
Copy link
Contributor Author

noamr commented May 4, 2022

I still don't feel comfortable discouraging 200 responses to range requests in HTML, when it isn't discouraged for HTTP in general. Maybe that part of the PR could be separated out and subject to a larger discussion with the HTTP + browser media community.

OK, I removed that line for now.
So right now this PR conforms to Chromium & Gecko and does not for WebKit.
I think it's better that it reflects the majority but I'd like to keep the out-of-consensus issue open. I personally think the points raised by the webkit folks are good ones, and in any case range mismatch is very rare in the wild.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to opening a dedicated issue about whether to add guidance (or update browsers) for the 200 responses.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming the tests verify that syntactically invalid range headers are ignored.

@noamr
Copy link
Contributor Author

noamr commented May 9, 2022

LGTM assuming the tests verify that syntactically invalid range headers are ignored.

Added requisite tests: web-platform-tests/wpt#33990

@domenic
Copy link
Member

domenic commented May 9, 2022

Looks like Firefox fails those; can you file a bug and update the OP?

@noamr
Copy link
Contributor Author

noamr commented May 9, 2022

Looks like Firefox fails those; can you file a bug and update the OP?

Done

@noamr
Copy link
Contributor Author

noamr commented May 17, 2022

Looks like Firefox fails those; can you file a bug and update the OP?

Done

@domenic, anything mssing?

@annevk
Copy link
Member

annevk commented May 17, 2022

I don't understand something. You remove the dependency on parsing the Content-Ranges header meaning you can no longer validate the range response, right?

However, https://bugzilla.mozilla.org/show_bug.cgi?id=1768448 suggests that Firefox not validating the range response according to Content-Ranges is problematic.

@noamr
Copy link
Contributor Author

noamr commented May 17, 2022

I don't understand something. You remove the dependency on parsing the Content-Ranges header meaning you can no longer validate the range response, right?

However, https://bugzilla.mozilla.org/show_bug.cgi?id=1768448 suggests that Firefox not validating the range response according to Content-Ranges is problematic.

Still parsing the header, but allowing 200 responses and responses that don't match the requested range. Syntactically invalid responses are still rejected.

@annevk
Copy link
Member

annevk commented May 17, 2022

How do they end up rejected if we don't look at the Content-Range header?

@noamr
Copy link
Contributor Author

noamr commented May 17, 2022

How do they end up rejected if we don't look at the Content-Range header?

I see what you mean, in one of the iterations we removed an important check. We have to check that the range is valid (even if not matching).
I'll revise.

@noamr
Copy link
Contributor Author

noamr commented May 18, 2022

How do they end up rejected if we don't look at the Content-Range header?

Revised, now extracting content-ranges is expected to at least succeed, even if the range itself is not validated. This aligns with the WPTs.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good after adding a note explaining the (somewhat strange) situation.

source Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants