Summary: | [Win] Fix Assert when handling Legible Output callbacks | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||
Component: | Media | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, dbates, eric.carlson, glenn, jer.noble, philipj, sergio, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 135173 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2014-07-15 15:37:32 PDT
Created attachment 234960 [details]
Patch
Created attachment 234961 [details]
Patch
Comment on attachment 234961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234961&action=review Thanks Brent! > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1534 > + RetainPtr<CFArrayRef> subtypes = createLegibleOutputSubtypes(); I think you want to adoptCF(createLegibleOutputSubtypes()) because the array already has a ref count of 1. Comment on attachment 234961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234961&action=review > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1495 > + formatTypes[0] = CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &webvtt); This CFNumber leaks. I suggest this instead: int webVTTInt = kCMSubtitleFormatType_WebVTT; RetainPtr<CFNumberRef> webVTTNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &webVTTInt)); CFTypeRef formatTypes[] = { webVTTNumber.get(); }; return adoptCF(CFArrayCreate(0, formatTypes, WTF_ARRAY_LENGT(formatTypes), &kCFTypeArrayCallBacks)); >> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1534 >> + RetainPtr<CFArrayRef> subtypes = createLegibleOutputSubtypes(); > > I think you want to adoptCF(createLegibleOutputSubtypes()) because the array already has a ref count of 1. That’s right, but it’s also better to have that function return a RetainPtr so the adoptCF is right next to the CFArrayCreate. Comment on attachment 234961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234961&action=review >> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1495 >> + formatTypes[0] = CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &webvtt); > > This CFNumber leaks. I suggest this instead: > > int webVTTInt = kCMSubtitleFormatType_WebVTT; > RetainPtr<CFNumberRef> webVTTNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &webVTTInt)); > CFTypeRef formatTypes[] = { webVTTNumber.get(); }; > return adoptCF(CFArrayCreate(0, formatTypes, WTF_ARRAY_LENGT(formatTypes), &kCFTypeArrayCallBacks)); Done! >>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1534 >>> + RetainPtr<CFArrayRef> subtypes = createLegibleOutputSubtypes(); >> >> I think you want to adoptCF(createLegibleOutputSubtypes()) because the array already has a ref count of 1. > > That’s right, but it’s also better to have that function return a RetainPtr so the adoptCF is right next to the CFArrayCreate. Done. Created attachment 235094 [details]
Patch
Comment on attachment 235094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235094&action=review > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:77 > +SOFT_LINK_DLL_IMPORT(CoreMedia, CMSampleBufferGetDataBuffer, CMBlockBufferRef, __cdecl, (CMSampleBufferRef sbuf), (sbuf)) This change isn’t mentioned explicitly in the change log, but it’s not yet compiling on EWS: 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2143: syntax error : missing ';' before '__cdecl' 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2065: 'CMSampleBufferRef' : undeclared identifier 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2146: syntax error : missing ')' before identifier 'sbuf' 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): warning C4229: anachronism used : modifiers on data are ignored 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2059: syntax error : ')' 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2059: syntax error : '__cdecl' 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2086: 'int CMBlockBufferRef' : redefinition Maybe they don’t have the correct version of CoreMedia.h? Comment on attachment 235094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235094&action=review review+ if you can get it to compile on EWS and bots and such > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1533 > + RetainPtr<CFArrayRef> subtypes = createLegibleOutputSubtypes(); > + m_legibleOutput = adoptCF(AVCFPlayerItemLegibleOutputCreateWithMediaSubtypesForNativeRepresentation(kCFAllocatorDefault, subtypes.get())); Not sure you really even need the local variable here. Putting createLegibleOutputSubtypes right into the expression might make the line excessively long, but otherwise it seems like an improvement. Created attachment 235251 [details]
Ignore. Accidental merge of two patches.
Comment on attachment 235094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235094&action=review >> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:77 >> +SOFT_LINK_DLL_IMPORT(CoreMedia, CMSampleBufferGetDataBuffer, CMBlockBufferRef, __cdecl, (CMSampleBufferRef sbuf), (sbuf)) > > This change isn’t mentioned explicitly in the change log, but it’s not yet compiling on EWS: > > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2143: syntax error : missing ';' before '__cdecl' > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2065: 'CMSampleBufferRef' : undeclared identifier > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2146: syntax error : missing ')' before identifier 'sbuf' > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): warning C4229: anachronism used : modifiers on data are ignored > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2059: syntax error : ')' > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2059: syntax error : '__cdecl' > 1>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(77): error C2086: 'int CMBlockBufferRef' : redefinition > > Maybe they don’t have the correct version of CoreMedia.h? Yes, it looks like the EWS bots are set up with an old set of headers. I'm trying to get confirmation on how these machines are configured. >> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1533 >> + m_legibleOutput = adoptCF(AVCFPlayerItemLegibleOutputCreateWithMediaSubtypesForNativeRepresentation(kCFAllocatorDefault, subtypes.get())); > > Not sure you really even need the local variable here. Putting createLegibleOutputSubtypes right into the expression might make the line excessively long, but otherwise it seems like an improvement. Sounds good to me! Created attachment 235307 [details]
Patch
Committed r171357: <http://trac.webkit.org/changeset/171357> Re-opened since this is blocked by bug 135173 Created attachment 235312 [details]
Add struct declarations to support building on EWS bots
Change landed as follows: 1. Bulk of change (building locally): <http://trac.webkit.org/changeset/171385> 2. Attempt forward declaration: <http://trac.webkit.org/changeset/171388> 3. Provide struct definition: <http://trac.webkit.org/changeset/171392> 4. Provide missing constant declaration: <http://trac.webkit.org/changeset/171404> Changes are in place and working on all bots. |