| Summary: | http streams don't always display video with AVFoundation backend | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | simon.fraser | ||||
| Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Mac (Intel) | ||||||
| OS: | Other | ||||||
| Attachments: |
|
||||||
|
Description
Eric Carlson
2011-03-27 22:15:50 PDT
Created attachment 87167 [details]
Proposed patch
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 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. (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. |