Bug 134946 – [Win] Fix Assert when handling Legible Output callbacks

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Brent Fulgham]

The changes in Bug 134178 introduced a Debug Assert in Windows, because the new InbandTextTrackPrivateAVF::processNativeSamples method is called on Windows, but is stubbed out as an 'ASSERT_NOT_REACHED'.

This code should not have been excluded on Windows. This patch allows Windows to use the same code path as Mac/iOS, and prevents the assertion from firing.

[Radar WebKit Bug Importer]

<rdar://problem/17687098>

[Brent Fulgham]

Created attachment 234960 [details]
Patch

[Brent Fulgham]

Created attachment 234961 [details]
Patch

[Eric Carlson]

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.

[Darin Adler]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 235094 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

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.

[Brent Fulgham]

Created attachment 235251 [details]
Ignore. Accidental merge of two patches.

[Brent Fulgham]

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!

[Brent Fulgham]

Created attachment 235307 [details]
Patch

[Brent Fulgham]

Committed r171357: <http://trac.webkit.org/changeset/171357>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 135173

[Brent Fulgham]

Created attachment 235312 [details]
Add struct declarations to support building on EWS bots

[Brent Fulgham]

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.