Bug 234940 - DataURLResourceMediaLoader decodes the URL repeatedly during a video playback
Summary: DataURLResourceMediaLoader decodes the URL repeatedly during a video playback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-06 17:16 PST by Peng Liu
Modified: 2022-01-18 12:42 PST (History)
5 users (show)

See Also:


Attachments
[fast-cq] Patch (2.92 KB, patch)
2022-01-06 17:37 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2022-01-06 17:16:49 PST
DataURLResourceMediaLoader decodes the URL repeatedly during a video playback
Comment 1 Peng Liu 2022-01-06 17:37:11 PST
Created attachment 448550 [details]
[fast-cq] Patch
Comment 2 Peng Liu 2022-01-06 17:38:06 PST
<rdar://83925935>
Comment 3 Jean-Yves Avenard [:jya] 2022-01-06 18:27:01 PST
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 ?
Comment 4 Peng Liu 2022-01-07 09:00:40 PST
(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.
Comment 5 Darin Adler 2022-01-11 11:19:59 PST
Comment on attachment 448550 [details]
[fast-cq] Patch

Can we find a way to test this?
Comment 6 Peng Liu 2022-01-11 15:22:37 PST
(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.
Comment 7 EWS 2022-01-11 15:26:24 PST
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].
Comment 8 Darin Adler 2022-01-11 16:34:13 PST
We have other tests for performance issues. I’m surprised to hear we can’t make a test for this one.
Comment 9 Peng Liu 2022-01-11 17:04:54 PST
(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?
Comment 10 Darin Adler 2022-01-11 17:41:00 PST
Yes. There are a few in LayoutTests/perf for example.
Comment 11 Peng Liu 2022-01-12 08:59:30 PST
(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.
Comment 12 Darin Adler 2022-01-12 09:59:01 PST
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.
Comment 13 Peng Liu 2022-01-18 12:42:07 PST
(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