Bug 234940 – DataURLResourceMediaLoader decodes the URL repeatedly during a video playback

• Component: New Bugs
• Status: RESOLVED FIXED

[Peng Liu]

DataURLResourceMediaLoader decodes the URL repeatedly during a video playback

[Peng Liu]

Created attachment 448550 [details]
[fast-cq] Patch

[Peng Liu]

<rdar://83925935>

[Jean-Yves Avenard [:jya]]

Comment on attachment 448550 [details]
[fast-cq] Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:365
> +        if (!m_dataURLMediaLoader && [contentInfo respondsToSelector:@selector(setEntireLengthAvailableOnDemand:)])

couldn't we cache the URL decoding instead and re-use that ?

[Peng Liu]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Comment on attachment 448550 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448550&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:365
> > +        if (!m_dataURLMediaLoader && [contentInfo respondsToSelector:@selector(setEntireLengthAvailableOnDemand:)])
> 
> couldn't we cache the URL decoding instead and re-use that ?

Actually, this is what this patch does. AVFoundation caches the decoded result internally. Some refactoring is needed if we want to cache the result in WebKit.

[Darin Adler]

Comment on attachment 448550 [details]
[fast-cq] Patch

Can we find a way to test this?

[Peng Liu]

(In reply to Darin Adler from comment #5)
> Comment on attachment 448550 [details]
> Patch
> 
> Can we find a way to test this?

This patch fixes a performance issue. I manually tested it. But I am not sure we can add a reliable regression test for it.

[EWS]

Committed r287899 (245936@main): <https://commits.webkit.org/245936@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448550 [details].

[Darin Adler]

We have other tests for performance issues. I’m surprised to hear we can’t make a test for this one.

[Peng Liu]

(In reply to Darin Adler from comment #8)
> We have other tests for performance issues. I’m surprised to hear we can’t
> make a test for this one.

Do you mean regression tests which run automatically on bots?

[Darin Adler]

Yes. There are a few in LayoutTests/perf for example.

[Peng Liu]

(In reply to Darin Adler from comment #10)
> Yes. There are a few in LayoutTests/perf for example.

Thanks! These tests look useful!

However, the test for this patch is more difficult. Because the performance issue is related to AVFoundation's internal behavior, and there is no obvious way to get the performance difference (before and after applying this patch) from JS code.

[Darin Adler]

I’m willing to accept no for an answer.

But I really don’t understand why we can’t create a test; every kind of test we have originally did not exist, and was invented because we needed to protect the performance or reliability of WebKit in the long run. Presumably we can make a huge stress test case where the performance impact is very obvious, and make a way to run it.

Given the fix here is subtle, and depends on both WebKit and AVFoundation, I think it’s really risky to just make the fix without creating a regression test.

[Peng Liu]

(In reply to Darin Adler from comment #12)
> I’m willing to accept no for an answer.
> 
> But I really don’t understand why we can’t create a test; every kind of test
> we have originally did not exist, and was invented because we needed to
> protect the performance or reliability of WebKit in the long run. Presumably
> we can make a huge stress test case where the performance impact is very
> obvious, and make a way to run it.
> 
> Given the fix here is subtle, and depends on both WebKit and AVFoundation, I
> think it’s really risky to just make the fix without creating a regression
> test.

I am working on a regression test now. Eric and Jer provided some good suggestions.
https://bugs.webkit.org/show_bug.cgi?id=235325