DataURLResourceMediaLoader decodes the URL repeatedly during a video playback
Created attachment 448550 [details] [fast-cq] Patch
<rdar://83925935>
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 ?
(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 on attachment 448550 [details] [fast-cq] Patch Can we find a way to test this?
(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.
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].
We have other tests for performance issues. I’m surprised to hear we can’t make a test for this one.
(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?
Yes. There are a few in LayoutTests/perf for example.
(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.
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.
(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