Bug 145185 – [iOS] When viewing an MJPEG as the main resource, only the first frame paints

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jon Honeycutt]

MJPEG streams (e.g. from internet cameras) when viewed as the main resource only paint the first frame, then stop painting although the site continues to appear to be loading data.

<rdar://problem/20124694>

[Jon Honeycutt]

Created attachment 253403 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 253403 [details]
Patch

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

Hard to review without a description of what the change is trying to do.

> Source/WebCore/ChangeLog:11
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * loader/DocumentLoader.cpp:

Please summarize the fix here.

> Source/WebKit2/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:

And here.

[Jon Honeycutt]

Created attachment 253458 [details]
Patch

[Darin Adler]

Comment on attachment 253458 [details]
Patch

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

Everything here is iOS-only, but the bug title doesn’t have [iOS] and it should!

> Source/WebCore/loader/FrameLoaderClient.h:203
> +        virtual void willReplaceMultipartContent() = 0;
> +        virtual void didReplaceMultipartContent() = 0;

Broke Windows build:

..\..\win\WebFrame.cpp(1053): error C2259: 'WebFrameLoaderClient' : cannot instantiate abstract class [C:\cygwin\home\buildbot\WebKit\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          due to following members:
          'void WebCore::FrameLoaderClient::willReplaceMultipartContent(void)' : is abstract
          C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Include\WebCore/FrameLoaderClient.h(202) : see declaration of 'WebCore::FrameLoaderClient::willReplaceMultipartContent'
          'void WebCore::FrameLoaderClient::didReplaceMultipartContent(void)' : is abstract
          C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Include\WebCore/FrameLoaderClient.h(203) : see declaration of 'WebCore::FrameLoaderClient::didReplaceMultipartContent'

> Source/WebCore/page/FrameView.cpp:419
> +    // Re-enable tile updates that were disabled in clear().
> +    if (LegacyTileCache* tileCache = legacyTileCache())
> +        tileCache->setTilingMode(LegacyTileCache::Normal);

I worry about this code running for subframes.

> Source/WebCore/page/FrameView.h:349
> +#if PLATFORM(IOS)
> +    WEBCORE_EXPORT void didReplaceMultipartContent();
> +#endif

Sad to be adding this. Not forward looking to have the existing hack be so platform-specific and to live in FrameView but also be spread across WebCore and WebKit.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1005
> +    if (FrameView *view = core(m_webFrame.get())->view())

Do we have a guarantee that m_webFrame is non-null?

Is it possible that view() will return null but the FrameView is still around, just not pointed to by this frame?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4583
> +    // Restore the previous exposed content rect so that it remains fixed when replacing content
> +    // from multipart/x-mixed-replace streams.
> +    m_drawingArea->setExposedContentRect(m_previousExposedContentRect);

Are these always guaranteed to come in pairs without anything else happening in between?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:257
> +    void willReplaceMultipartContent(WebFrame*);
> +    void didReplaceMultipartContent(WebFrame*);

Should take WebFrame&, not WebFrame*.

[Jon Honeycutt]

(In reply to comment #4)
> Comment on attachment 253458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253458&action=review
> 
> Everything here is iOS-only, but the bug title doesn’t have [iOS] and it
> should!
> 
> > Source/WebCore/loader/FrameLoaderClient.h:203
> > +        virtual void willReplaceMultipartContent() = 0;
> > +        virtual void didReplaceMultipartContent() = 0;
> 
> Broke Windows build:
> 

Fixed.


> > Source/WebCore/page/FrameView.cpp:419
> > +    // Re-enable tile updates that were disabled in clear().
> > +    if (LegacyTileCache* tileCache = legacyTileCache())
> > +        tileCache->setTilingMode(LegacyTileCache::Normal);
> 
> I worry about this code running for subframes.
> 
> > Source/WebCore/page/FrameView.h:349
> > +#if PLATFORM(IOS)
> > +    WEBCORE_EXPORT void didReplaceMultipartContent();
> > +#endif
> 
> Sad to be adding this. Not forward looking to have the existing hack be so
> platform-specific and to live in FrameView but also be spread across WebCore
> and WebKit.
> 
> > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1005
> > +    if (FrameView *view = core(m_webFrame.get())->view())
> 
> Do we have a guarantee that m_webFrame is non-null?
> 
> Is it possible that view() will return null but the FrameView is still
> around, just not pointed to by this frame?
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4583
> > +    // Restore the previous exposed content rect so that it remains fixed when replacing content
> > +    // from multipart/x-mixed-replace streams.
> > +    m_drawingArea->setExposedContentRect(m_previousExposedContentRect);
> 
> Are these always guaranteed to come in pairs without anything else happening
> in between?
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:257
> > +    void willReplaceMultipartContent(WebFrame*);
> > +    void didReplaceMultipartContent(WebFrame*);
> 
> Should take WebFrame&, not WebFrame*.

[Jon Honeycutt]

Whoops, hit enter too soon.


(In reply to comment #4)
> Comment on attachment 253458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253458&action=review
> 
> Everything here is iOS-only, but the bug title doesn’t have [iOS] and it
> should!

Fixed.

> 
> > Source/WebCore/loader/FrameLoaderClient.h:203
> > +        virtual void willReplaceMultipartContent() = 0;
> > +        virtual void didReplaceMultipartContent() = 0;
> 
> Broke Windows build:

Fixed.

> 
> > Source/WebCore/page/FrameView.cpp:419
> > +    // Re-enable tile updates that were disabled in clear().
> > +    if (LegacyTileCache* tileCache = legacyTileCache())
> > +        tileCache->setTilingMode(LegacyTileCache::Normal);
> 
> I worry about this code running for subframes.

legacyTileCache() returns null for subframes. It has a comment saying that this is so callers don't need to check whether this is the main frame, so I don't expect this to change without all call sites being updated.

> 
> > Source/WebCore/page/FrameView.h:349
> > +#if PLATFORM(IOS)
> > +    WEBCORE_EXPORT void didReplaceMultipartContent();
> > +#endif
> 
> Sad to be adding this. Not forward looking to have the existing hack be so
> platform-specific and to live in FrameView but also be spread across WebCore
> and WebKit.

Yes, it's a shame that this exists in WebCore. I'll look into whether we can move this to WebKit.

> 
> > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1005
> > +    if (FrameView *view = core(m_webFrame.get())->view())
> 
> Do we have a guarantee that m_webFrame is non-null?

Yes, according to Brady, m_webFrame should never be null.

> 
> Is it possible that view() will return null but the FrameView is still
> around, just not pointed to by this frame?

I'm not sure. I believe that a DocumentLoader will be detached from its frame before a frame is detached from its view, and this point won't be reached when no FrameView exists. Are you concerned that there may be a lingering FrameView whose tile cache remains disabled because no Frame points to it? If the Frame detached its FrameView while the cache was disabled, there shouldn't be anything to paint, because the FrameView was cleared.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4583
> > +    // Restore the previous exposed content rect so that it remains fixed when replacing content
> > +    // from multipart/x-mixed-replace streams.
> > +    m_drawingArea->setExposedContentRect(m_previousExposedContentRect);
> 
> Are these always guaranteed to come in pairs without anything else happening
> in between?

Yes, these are both called below didReceiveResponse.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:257
> > +    void willReplaceMultipartContent(WebFrame*);
> > +    void didReplaceMultipartContent(WebFrame*);
> 
> Should take WebFrame&, not WebFrame*.

Fixed.

Thanks for the review!

[Jon Honeycutt]

Committed r184947: <http://trac.webkit.org/changeset/184947>