Synthesize range responses if needed in WebCoreNSURLSession

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Synthesize range responses if needed in WebCoreNSURLSession

[Alex Christensen]

Created attachment 418606 [details]
Patch

[Geoffrey Garen]

Comment on attachment 418606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418606&action=review

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:983
>  - (void)resource:(PlatformMediaResource&)resource receivedData:(const char*)data length:(int)length
>  {
> +    if (_buffer) {
> +        _buffer->append(data, length);
> +        return;
> +    }

Should we consider synthesizing a receivedData here too, in case there's been a range request within the received range, and loadFinished won't happen for a while?

[Alex Christensen]

Comment on attachment 418606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418606&action=review

> Source/WebCore/ChangeLog:9
> +        This first iteration is awful.  Don't approve it.  It downloads the entire video anytime any range is requested.

This suggests there's some serious room for improvement.

[Geoffrey Garen]

Got it.

[Alexey Proskuryakov]

rdar://problem/20696449&71729210 :)

[Alex Christensen]

Created attachment 418800 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/73959759>

[Alex Christensen]

Created attachment 419520 [details]
Patch

[Alex Christensen]

Created attachment 419522 [details]
Patch

[Alex Christensen]

Created attachment 419523 [details]
Patch

[Alex Christensen]

Created attachment 419524 [details]
Patch

[Sam Weinig]

Neat! Does this fix the issue on bugs.webkit.org not working with video attachments???

[Alex Christensen]

Comment on attachment 419524 [details]
Patch

It does!

[Geoffrey Garen]

Comment on attachment 419524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419524&action=review

r=me -- is the + 1 thing a bug?

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:127
> +            size_t bytesFromThisViewToDeliver = std::min(bufferView.size(), range.end() - byteIndex + 1);

Why + 1? (Typically, end - index is the right math for computing the size of a range.)

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:203
> +    class Client : public PlatformMediaResourceClient {

While it's true that we don't need access to this class declaration outside this scope, I think it would be more readable to declare it at file level. Maybe name it RangeResponsePlatformMediaResourceClient.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:210
> +        // These methods should have been called before changing the client to this.

We might as well make these release asserts, since they'll never be called.

[Alex Christensen]

(In reply to Geoffrey Garen from comment #14)
> Comment on attachment 419524 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419524&action=review
> 
> r=me -- is the + 1 thing a bug?
> 
> > Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:127
> > +            size_t bytesFromThisViewToDeliver = std::min(bufferView.size(), range.end() - byteIndex + 1);
> 
> Why + 1? (Typically, end - index is the right math for computing the size of
> a range.)
With HTTP ranges, you need to go to and including the end.  For example, if you have a request with a header field like this:
Range: bytes=0-1
then you need to deliver two bytes, byte zero and byte one.  If you don't, then our media implementation detects improper HTTP range support and fails to play the video.

[Alex Christensen]

Comment on attachment 419524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419524&action=review

>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:203
>> +    class Client : public PlatformMediaResourceClient {
> 
> While it's true that we don't need access to this class declaration outside this scope, I think it would be more readable to declare it at file level. Maybe name it RangeResponsePlatformMediaResourceClient.

It needs to access private members of RangeResponseGenerator.  I'm going to make it a nested class outside the function to get all the benefits of scope, private access, and readability.

>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:210
>> +        // These methods should have been called before changing the client to this.
> 
> We might as well make these release asserts, since they'll never be called.

Sure.

[Alex Christensen]

Created attachment 419750 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r272603

[Alex Christensen]

This was also rdar://problem/73575468

[Darin Adler]

Comment on attachment 419750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419750&action=review

Apparently landed already, but I have a few coding style comments.

> Source/WebCore/platform/network/ParsedRequestRange.cpp:33
> +Optional<ParsedRequestRange> ParsedRequestRange::parse(const String& input)

This should take a StringView. This will naturally make the implementation more efficient.

> Source/WebCore/platform/network/ParsedRequestRange.cpp:46
> +    // FIXME: Add StringView.toUInt64Strict to avoid these unnecessary String copies.

If we don’t want to put the StringView changes into the same patch, it’s still straightforward to avoid the unnecessary string copies by calling using an if (is8Bit()) call and calling charactersToUInt64Strict directly, without adding to StringView. I would prefer that. Or you can even put the helper function in this file and just have the FIXME be about moving the function from this file to StringView.h/cpp.

> Source/WebCore/platform/network/ParsedRequestRange.h:32
> +class ParsedRequestRange {

Seems to me this it would be nicer to have this be a struct rather than a class. In contexts where we want to emphasize that we don’t want to modify begin and end, we can use const. Rather than having a class where the const-ness is built in.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.h:50
> +    class MediaResourceClient;

Just want to double check that this needs to be a member. Can just be in the .cpp file, but I think being a member gives it access to private stuff. Leaving it out of the header is nicer for future refactoring, so want to confirm it’s worthwhile to have it here.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.h:56
> +    HashMap<URL, std::unique_ptr<Data>> m_map;

Should consider whether we want to use URLs or URL strings to key this map. Is it important to use the extra memory to keep the parsed URL character offsets in the map? If not, I don’t see much downside to key based on the URL strings instead of the actual URL objects. But maybe I am missing some advantage that is worth the considerable extra memory use.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:43
> +    WTF_MAKE_FAST_ALLOCATED;
> +public:

We have a WTF_MAKE_STRUCT_FAST_ALLOCATED macro that avoids the need for this awkward "public".

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:49
> +    struct TaskData : public CanMakeWeakPtr<TaskData> {

I suggest omitting public when specifying base classes for structs; they are public by default. For classes we tend to be explicit about both public and private even though private is the default, but I think that’s because public inheritance is *so* much more common.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:51
> +        WTF_MAKE_FAST_ALLOCATED;
> +    public:

Here too.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:53
> +            : range(WTFMove(range))

I don’t think we need to use move semantics for a struct with two integers in it.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:154
> +    // FIXME: ResourceResponseBase::expectedContentLength() should return Optional<size_t> instead of us doing this check here.

I agree with this because NSURLResponseUnknownLength is a platform-specific sentinel value that should not make its way into platform-independent code like ResourceResponseBase.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:198
> +        RELEASE_ASSERT_NOT_REACHED();

I’d love to find an adjustment we could make to design this out. I think it’s the classic Liskov Substitution Principle problem.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:154
> +@interface WebCoreNSURLSessionDataTask (WebKitInternal)

Does this belong in a different header? Is this exposing internals of NSURLSession? If so, I think it should go into an "SPI" header, even though SPI is not quite the right term.

Why the category name WebKitInternal? What does that mean exactly; for use in WebKit internally, or defined by WebKit for user internally? Maybe omitting the name is better?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:126
> +        "video.autoplay = 1;"

I’d leave out the spaces here to match the lines above.

> Tools/TestWebKitAPI/cocoa/HTTPServer.mm:215
> +static void appendToVector(Vector<uint8_t>& vector, const String& string)

I think this function needs UTF8 in its name.

> Tools/TestWebKitAPI/cocoa/HTTPServer.mm:217
> +    auto utf8 = string.utf8();

Very unfortunate to make so many copies.

[Alex Christensen]

Comment on attachment 419750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419750&action=review

>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.h:50
>> +    class MediaResourceClient;
> 
> Just want to double check that this needs to be a member. Can just be in the .cpp file, but I think being a member gives it access to private stuff. Leaving it out of the header is nicer for future refactoring, so want to confirm it’s worthwhile to have it here.

It needs access to RangeResponseGenerator's private members, and I like the scoping of nested classes here.

>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.h:56
>> +    HashMap<URL, std::unique_ptr<Data>> m_map;
> 
> Should consider whether we want to use URLs or URL strings to key this map. Is it important to use the extra memory to keep the parsed URL character offsets in the map? If not, I don’t see much downside to key based on the URL strings instead of the actual URL objects. But maybe I am missing some advantage that is worth the considerable extra memory use.

URL strings would save memory.  That's true.

>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:43
>> +public:
> 
> We have a WTF_MAKE_STRUCT_FAST_ALLOCATED macro that avoids the need for this awkward "public".

That's exactly what I need here.  Thanks.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:154
>> +@interface WebCoreNSURLSessionDataTask (WebKitInternal)
> 
> Does this belong in a different header? Is this exposing internals of NSURLSession? If so, I think it should go into an "SPI" header, even though SPI is not quite the right term.
> 
> Why the category name WebKitInternal? What does that mean exactly; for use in WebKit internally, or defined by WebKit for user internally? Maybe omitting the name is better?

We do need to move this to a different header.  See rdar://problem/72317626

[Alex Christensen]

Comment on attachment 419750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419750&action=review

>> Source/WebCore/platform/network/ParsedRequestRange.cpp:46
>> +    // FIXME: Add StringView.toUInt64Strict to avoid these unnecessary String copies.
> 
> If we don’t want to put the StringView changes into the same patch, it’s still straightforward to avoid the unnecessary string copies by calling using an if (is8Bit()) call and calling charactersToUInt64Strict directly, without adding to StringView. I would prefer that. Or you can even put the helper function in this file and just have the FIXME be about moving the function from this file to StringView.h/cpp.

StringView.toUInt64Strict already exists.  Will adopt.

[Alex Christensen]

This also caused https://bugs.webkit.org/show_bug.cgi?id=221698 and has room for improvement and needs some more investigation as to why some videos still aren't playing.  Reverting.

[Alex Christensen]

Reopening because I reverted.

[Alex Christensen]

Created attachment 420405 [details]
Patch

[Alex Christensen]

Created attachment 420406 [details]
Patch

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[Alex Christensen]

http://trac.webkit.org/r272908

[Darin Adler]

Comment on attachment 420406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420406&action=review

Sorry to catch you after you landed. A few thoughts post-landing:

> Source/WebCore/ChangeLog:11
> +        Seeking is problematic, but at least we will try our best to play the video.

I would like to understand more about why "seeking is problematic".

> Source/WebCore/platform/network/ParsedRequestRange.cpp:36
> +    if (!input.startsWith(StringView("bytes="_s)))

Should just be able to write "bytes=" here, not StringView("bytes="_s).

> Source/WebCore/platform/network/ParsedRequestRange.cpp:40
> +    size_t begin { 0 };
> +    size_t end { 0 };

I suggest we define these below when we set them. Not important to have local variables above there that aren’t used.

    auto begin = *optionalBegin;

Or just dump the name optionalBegin entirely, name that variable something more like just "begin" and write *begin in a few places.

> Source/WebCore/platform/network/ParsedRequestRange.cpp:41
> +    size_t rangeBeginPosition = 6;

Maybe constexpr for this to make it clear that it’s a constant.

> Source/WebCore/platform/network/ParsedRequestRange.cpp:50
> +    begin = *optionalBegin;

Here we are chopping a uint64_t to a size_t; not quite right on 32-bit.

> Source/WebCore/platform/network/ParsedRequestRange.cpp:56
> +    end = *optionalEnd;

Here we are chopping a uint64_t to a size_t; not quite right on 32-bit.

> Source/WebCore/platform/network/ParsedRequestRange.h:29
> +#include <wtf/HashTraits.h>

Why are we including this header?

> Source/WebCore/platform/network/ParsedRequestRange.h:35
> +public:

No need for public in a struct.

> Source/WebCore/platform/network/ParsedRequestRange.h:37
> +    static Optional<ParsedRequestRange> parse(const String& string) { return parse(StringView(string)); }

Do we really need this? I’d expect everything would compile just fine without this, and we also would not have to include StringView.h if we didn’t have this code.

> Source/WebCore/platform/network/ParsedRequestRange.h:40
> +    const size_t begin { 0 };
> +    const size_t end { 0 };

I think these should be uint64_t; I understand that if the values are really large then the code won’t work, but parsing should not depend on 32 vs. 64-bit. I guess this mostly manipulates buffers in memory, which of course should use size_t. But there may be some point where larger numbers have to be checked for on 32-bit systems?

Also, I suggest we just make the whole object const, rather than having each member be const.

> Source/WebCore/platform/network/ParsedRequestRange.h:45
> +private:
> +    ParsedRequestRange(size_t begin, size_t end)
> +        : begin(begin)
> +        , end(end) { }

Can we do without this constructor? I don’t see a strong reason this needs to be private.

[Alex Christensen]

Thanks for the feedback.  I'll do a follow-up patch that incorporates most of it.  Here's some responses:

(In reply to Darin Adler from comment #29)
> Comment on attachment 420406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420406&action=review
> 
> Sorry to catch you after you landed. A few thoughts post-landing:
> 
> > Source/WebCore/ChangeLog:11
> > +        Seeking is problematic, but at least we will try our best to play the video.
> 
> I would like to understand more about why "seeking is problematic".
Seeking to near the end of a large video would cause a new range request to be issued for a range near the end, which would have to wait until the whole video is loaded to respond.  There's not much of a way around this, but Jer and I had discussed maybe preventing seeking to parts of the video that haven't loaded yet.

> > Source/WebCore/platform/network/ParsedRequestRange.cpp:36
> > +    if (!input.startsWith(StringView("bytes="_s)))
> 
> Should just be able to write "bytes=" here, not StringView("bytes="_s).
That would require the StringView(ASCIILiteral) constructor no longer being explicit.  I'm not sure why it is, but I felt like I didn't have the justification to change it.

> > Source/WebCore/platform/network/ParsedRequestRange.h:29
> > +#include <wtf/HashTraits.h>
> 
> Why are we including this header?
I needed it in an earlier iteration of this patch.  I can remove it.

> > Source/WebCore/platform/network/ParsedRequestRange.h:37
> > +    static Optional<ParsedRequestRange> parse(const String& string) { return parse(StringView(string)); }
> 
> Do we really need this? I’d expect everything would compile just fine
> without this, and we also would not have to include StringView.h if we
> didn’t have this code.
There are places where we pass in an NSString *, and there isn't a way to get a StringView out of an NSString * directly.  It's either this or an explicit String constructor at each call site to get the implicit StringView constructor.
 
> > Source/WebCore/platform/network/ParsedRequestRange.h:45
> > +private:
> > +    ParsedRequestRange(size_t begin, size_t end)
> > +        : begin(begin)
> > +        , end(end) { }
> 
> Can we do without this constructor? I don’t see a strong reason this needs
> to be private.
I wanted the only way to make a ParsedRequestRange be parsing one, which would return nullopt if it failed.  That's not too important, and I could just remove this for elegance.

[Alex Christensen]

Reverted in r273033

[Ryan Haddad]

As an additional note before this gets re-landed, WebKitAPI.MediaLoading.RangeRequestSynthesisWithContentLength was a flaky failure on Big Sur release bots https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.MediaLoading.RangeRequestSynthesisWithContentLength

[Alex Christensen]

Off again in http://trac.webkit.org/r273081

[Alex Christensen]

And off again in http://trac.webkit.org/r273263

[Alex Christensen]

And on again in https://trac.webkit.org/changeset/273287/webkit