Bug 57203 - http streams don't always display video with AVFoundation backend
Summary: http streams don't always display video with AVFoundation backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) Other
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2011-03-27 22:15 PDT by Eric Carlson
Modified: 2011-03-28 15:14 PDT (History)
0 users

See Also:


Attachments
Proposed patch (18.87 KB, patch)
2011-03-28 11:12 PDT, Eric Carlson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2011-03-27 22:15:50 PDT
http streams, live or stored, don't always display video when displayed with the new AVFoundation backend.
Comment 1 Eric Carlson 2011-03-28 11:12:03 PDT
Created attachment 87167 [details]
Proposed patch
Comment 2 Eric Carlson 2011-03-28 11:24:27 PDT
rdar://9172549
Comment 3 Simon Fraser (smfr) 2011-03-28 11:32:36 PDT
Comment on attachment 87167 [details]
Proposed patch

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

> Source/WebCore/platform/graphics/MediaPlayer.h:322
> +    bool m_isAllowedToRender;

'allowed' is a bit vague here. Allowed by whom? By MediaPlayer, or by the underlying framework?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.h:113
> +    static const NSArray* assetMetadataKeyNames();
> +    static const NSArray* itemKVOProperties();

I think these should just be static functions in the impl. file.
Comment 4 Darin Adler 2011-03-28 11:33:57 PDT
Comment on attachment 87167 [details]
Proposed patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:444
> -    if (isReadyForVideoSetup() && !hasSetUpVideoRendering())
> +    if (isReadyForVideoSetup() && currentRenderingMode() != preferredRenderingMode())

This looks fine, but I have no understanding of the reason for the change. The change log says what changed, but not why, and similarly the code does not say why. I’m not asking for a comment, just noting that there’s no way for me to verify this is a good change.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:156
> +        const NSArray *keys = itemKVOProperties();
> +        for (NSString *keyName in keys)

No need for a local variable here.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:264
> +        const NSArray *keys = itemKVOProperties();
> +        for (NSString *keyName in keys)

No need for a local variable here.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:676
> +const NSArray* MediaPlayerPrivateAVFoundationObjC::itemKVOProperties()

With an Objective-C type I don’t think the const does any good here other than requiring you to write const elsewhere. You can still call any method on such a pointer. Immutability in Objective-C and Foundation is a property of the class, not the constness of the pointer.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:680
> +    static NSArray* keys;
> +    if (!keys) {
> +        keys = [[NSArray alloc] initWithObjects: @"presentationSize",

This can just be static NSArray * keys = ... because C++ already handles the one time initialization for us.

On a separate note, I am worried that this code will not work properly under GC. I seem to recall that when global variables were in C++ functions that we had to do an explicit CFRetain. Tim Hatcher or Mark Rowe might remember.

Also we normally don’t put spaces after colons like this one.

We should be consistent about where we put the "*" for NSArray.
Comment 5 Eric Carlson 2011-03-28 15:14:27 PDT
(In reply to comment #4)
> (From update of attachment 87167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87167&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:444
> > -    if (isReadyForVideoSetup() && !hasSetUpVideoRendering())
> > +    if (isReadyForVideoSetup() && currentRenderingMode() != preferredRenderingMode())
> 
> This looks fine, but I have no understanding of the reason for the change. The change log says what changed, but not why, and similarly the code does not say why. I’m not asking for a comment, just noting that there’s no way for me to verify this is a good change.
> 
  Good point, I added a more thorough explanation to the ChangeLog.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:156
> > +        const NSArray *keys = itemKVOProperties();
> > +        for (NSString *keyName in keys)
> 
> No need for a local variable here.
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:264
> > +        const NSArray *keys = itemKVOProperties();
> > +        for (NSString *keyName in keys)
> 
> No need for a local variable here.
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:676
> > +const NSArray* MediaPlayerPrivateAVFoundationObjC::itemKVOProperties()
> 
> With an Objective-C type I don’t think the const does any good here other than requiring you to write const elsewhere. You can still call any method on such a pointer. Immutability in Objective-C and Foundation is a property of the class, not the constness of the pointer.
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:680
> > +    static NSArray* keys;
> > +    if (!keys) {
> > +        keys = [[NSArray alloc] initWithObjects: @"presentationSize",
> 
> This can just be static NSArray * keys = ... because C++ already handles the one time initialization for us.
> 
> On a separate note, I am worried that this code will not work properly under GC. I seem to recall that when global variables were in C++ functions that we had to do an explicit CFRetain. Tim Hatcher or Mark Rowe might remember.
> 
  Some code in WebCore does do an extra CFRetain() after allocating an NS type and some does not, but Anders says it should not be necessary as long as the variable is declared static.

> Also we normally don’t put spaces after colons like this one.
> 
> We should be consistent about where we put the "*" for NSArray.
>
  Oops, fixed.
Comment 6 Eric Carlson 2011-03-28 15:14:38 PDT
http://trac.webkit.org/changeset/82165