Bug 163308 - REGRESSION (r206025): All YouTube videos play with black bars on all four sides
Summary: REGRESSION (r206025): All YouTube videos play with black bars on all four sides
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: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-11 17:09 PDT by Jer Noble
Modified: 2016-10-19 21:06 PDT (History)
1 user (show)

See Also:


Attachments
Patch (9.17 KB, patch)
2016-10-12 09:16 PDT, Jer Noble
darin: review+
Details | Formatted Diff | Diff
Patch for landing (10.60 KB, patch)
2016-10-13 13:15 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2016-10-11 17:09:31 PDT
REGRESSION (r206025): All YouTube videos play with black bars on all four sides
Comment 1 Jer Noble 2016-10-11 17:14:11 PDT
rdar://problem/28613862
Comment 2 Jer Noble 2016-10-12 09:16:08 PDT
Created attachment 291360 [details]
Patch
Comment 3 Darin Adler 2016-10-13 12:37:14 PDT
Comment on attachment 291360 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:692
>          RetainPtr<id> observer = weakThis->m_sizeChangeObservers.takeFirst();

What is going on here? How is it valuable to set a local variable that is then never used?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:960
> +    m_cachedSize = FloatSize();

I think { } is nicer here than FloatSize().

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:988
>              m_cachedSize = formatSize;

What guarantees this new size is not zero?
Comment 4 Jer Noble 2016-10-13 12:58:00 PDT
(In reply to comment #3)
> Comment on attachment 291360 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291360&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:692
> >          RetainPtr<id> observer = weakThis->m_sizeChangeObservers.takeFirst();
> 
> What is going on here? How is it valuable to set a local variable that is
> then never used?

I looked back through this file's history, hoping that I'd find some case where I'd previously used the results of observer.  Alas, it's never been used. I'll take care of this in a follow-up.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:960
> > +    m_cachedSize = FloatSize();
> 
> I think { } is nicer here than FloatSize().

Ok!

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:988
> >              m_cachedSize = formatSize;
> 
> What guarantees this new size is not zero?

Nothing, and good point. Maybe I'll make this an Optional<FloatSize> and use !m_cachedSize instead of .isZero().
Comment 5 Jer Noble 2016-10-13 13:15:09 PDT
Created attachment 291509 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-10-19 20:12:57 PDT
Comment on attachment 291509 [details]
Patch for landing

Clearing flags on attachment: 291509

Committed r207584: <http://trac.webkit.org/changeset/207584>