Bug 134178 – [Mac] process raw VTT in-band captions

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

[Eric Carlson]

Configure AVFoundation to deliver raw WebVTT captions.

[Eric Carlson]

<rdar://problem/13237650>

[Eric Carlson]

Created attachment 233577 [details]
Patch for the bots to chew on.

[Eric Carlson]

Created attachment 233578 [details]
Rebased patch

[WebKit Commit Bot]

Attachment 233578 [details] did not pass style-queue:


ERROR: Source/WebCore/html/track/InbandGenericTextTrack.cpp:221:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/html/track/InbandGenericTextTrack.cpp:238:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/loader/TextTrackLoader.cpp:219:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObjC.mm:79:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObjC.mm:80:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/ISOVTTCue.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/ISOVTTCue.cpp:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:81:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:98:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/html/track/VTTCue.h:55:  The parameter name "document" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/html/track/VTTCue.h:55:  The parameter name "cue" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/ISOVTTCue.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:113:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 233608 [details]
Updated patch.

[WebKit Commit Bot]

Attachment 233608 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/ISOVTTCue.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 233611 [details]
Another patch.

[Brent Fulgham]

The build failure doesn't seem to make it into the log EWS shows. It's not happy with the SOFT_LINK macro on Windows:

20>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(67): warning C4003: not enough actual parameters for macro 'SOFT_LINK'
20>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(67): error C2146: syntax error : missing ')' before identifier 'sbuf'
20>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(67): error C2146: syntax error : missing ';' before identifier 'sbuf'
20>..\platform\graphics\avfoundation\InbandTextTrackPrivateAVF.cpp(67): error C2377: 'CMSampleBufferRef' : redefinition; typedef cannot be overloaded with any other symbol
20>          C:\AppleInternal\include\CoreMedia/CMSampleBuffer.h(409) : see declaration of 'CMSampleBufferRef'

SOFT_LINK on Windows looks like:

#define SOFT_LINK(library, functionName, resultType, callingConvention, parameterDeclarations, parameterNames) \

On Windows, these are defined in "CoreMediaSoftLinking.h".  I'll tweak the patch and upload a revision.

[Brent Fulgham]

Created attachment 233618 [details]
Eric's Patch Tweaked for Windows Build

[Jer Noble]

Comment on attachment 233618 [details]
Eric's Patch Tweaked for Windows Build

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

r=me, with nits.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1615
> -		498770E91242C535002226BA /* Shader.h in Headers */ = {isa = PBXBuildFile; fileRef = 498770D01242C535002226BA /* Shader.h */; };
> +		498770E91242C535002226BA /* (null) in Headers */ = {isa = PBXBuildFile; };

This looks like an inadvertent change.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-15810
> -				498770D01242C535002226BA /* Shader.h */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25766
> -				498770E91242C535002226BA /* Shader.h in Headers */,
> +				498770E91242C535002226BA /* (null) in Headers */,

Ditto.

> Source/WebCore/html/track/InbandGenericTextTrack.cpp:249
> +WebVTTParser& InbandGenericTextTrack::parser()
> +{
> +    if (!m_webVTTParser)
> +        m_webVTTParser = std::make_unique<WebVTTParser>(static_cast<WebVTTParserClient*>(this), scriptExecutionContext());
> +    return *m_webVTTParser;
> +}
> +
> +void InbandGenericTextTrack::parseWebVTTCueData(InbandTextTrackPrivate* trackPrivate, const ISOWebVTTCue& cueData)
> +{
> +    ASSERT_UNUSED(trackPrivate, trackPrivate == m_private);
> +    parser().parseCueData(cueData);
> +}
> +
> +void InbandGenericTextTrack::parseWebVTTFileHeader(InbandTextTrackPrivate* trackPrivate, String header)
> +{
> +    ASSERT_UNUSED(trackPrivate, trackPrivate == m_private);
> +    parser().parseFileHeader(header);
> +}
> +
> +void InbandGenericTextTrack::newCuesParsed()
> +{
> +    Vector<RefPtr<WebVTTCueData>> cues;
> +    parser().getNewCues(cues);
> +
> +    for (auto& cueData : cues) {
> +        RefPtr<VTTCue> vttCue = VTTCue::create(*scriptExecutionContext(), *cueData);
> +
> +        if (hasCue(vttCue.get(), TextTrackCue::IgnoreDuration)) {
> +            LOG(Media, "InbandGenericTextTrack::newCuesParsed ignoring already added cue: start=%.2f, end=%.2f, content=\"%s\"\n", vttCue->startTime(), vttCue->endTime(), vttCue->text().utf8().data());
> +            return;
> +        }
> +        addCue(vttCue.release(), ASSERT_NO_EXCEPTION);
> +    }
> +}
> +
> +#if ENABLE(WEBVTT_REGIONS)
> +void InbandGenericTextTrack::newRegionsParsed()
> +{
> +    Vector<RefPtr<VTTRegion>> newRegions;
> +    parser().getNewRegions(newRegions);
> +
> +    for (auto& region : newRegions) {
> +        region->setTrack(this);
> +        regions()->add(region);
> +    }
> +}
> +#endif
> +
> +void InbandGenericTextTrack::fileFailedToParse()
> +{
> +    LOG(Media, "Error parsing WebVTT stream.");
> +}
> +

This looks like code duplicated from InbandWebVTTTextTrack.cpp.  Is there any way to unify those two implementations?

> Source/WebCore/platform/graphics/ISOVTTCue.cpp:145
> +        else
> +            LOG(Media, "ISOWebVTTCue::ISOWebVTTCue - skipping box id = \"%s\", size = %zu", boxType.utf8().data(), boxSize);

Should this be an ASSERT_UNREACHED, or do we expect unexpected box types?

> Source/WebCore/platform/graphics/ISOVTTCue.h:38
> +// A ISOBox represents a ISO-BMFF box. Data in the structure is big-endian. The layout of the data structure as follows:

nit: "An ISOBox"

[Eric Carlson]

(In reply to comment #10)
> (From update of attachment 233618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233618&action=review
> 
> r=me, with nits.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:1615
> > -		498770E91242C535002226BA /* Shader.h in Headers */ = {isa = PBXBuildFile; fileRef = 498770D01242C535002226BA /* Shader.h */; };
> > +		498770E91242C535002226BA /* (null) in Headers */ = {isa = PBXBuildFile; };
> 
> This looks like an inadvertent change.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-15810
> > -				498770D01242C535002226BA /* Shader.h */,
> 
> Ditto.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25766
> > -				498770E91242C535002226BA /* Shader.h in Headers */,
> > +				498770E91242C535002226BA /* (null) in Headers */,
> 
> Ditto.
> 

All fixed.

> > Source/WebCore/html/track/InbandGenericTextTrack.cpp:249
> > +WebVTTParser& InbandGenericTextTrack::parser()
> > +{
> > +    if (!m_webVTTParser)
> > +        m_webVTTParser = std::make_unique<WebVTTParser>(static_cast<WebVTTParserClient*>(this), scriptExecutionContext());
> > +    return *m_webVTTParser;
> > +}
> > +
> > +void InbandGenericTextTrack::parseWebVTTCueData(InbandTextTrackPrivate* trackPrivate, const ISOWebVTTCue& cueData)
> > +{
> > +    ASSERT_UNUSED(trackPrivate, trackPrivate == m_private);
> > +    parser().parseCueData(cueData);
> > +}
> > +
> > +void InbandGenericTextTrack::parseWebVTTFileHeader(InbandTextTrackPrivate* trackPrivate, String header)
> > +{
> > +    ASSERT_UNUSED(trackPrivate, trackPrivate == m_private);
> > +    parser().parseFileHeader(header);
> > +}
> > +
> > +void InbandGenericTextTrack::newCuesParsed()
> > +{
> > +    Vector<RefPtr<WebVTTCueData>> cues;
> > +    parser().getNewCues(cues);
> > +
> > +    for (auto& cueData : cues) {
> > +        RefPtr<VTTCue> vttCue = VTTCue::create(*scriptExecutionContext(), *cueData);
> > +
> > +        if (hasCue(vttCue.get(), TextTrackCue::IgnoreDuration)) {
> > +            LOG(Media, "InbandGenericTextTrack::newCuesParsed ignoring already added cue: start=%.2f, end=%.2f, content=\"%s\"\n", vttCue->startTime(), vttCue->endTime(), vttCue->text().utf8().data());
> > +            return;
> > +        }
> > +        addCue(vttCue.release(), ASSERT_NO_EXCEPTION);
> > +    }
> > +}
> > +
> > +#if ENABLE(WEBVTT_REGIONS)
> > +void InbandGenericTextTrack::newRegionsParsed()
> > +{
> > +    Vector<RefPtr<VTTRegion>> newRegions;
> > +    parser().getNewRegions(newRegions);
> > +
> > +    for (auto& region : newRegions) {
> > +        region->setTrack(this);
> > +        regions()->add(region);
> > +    }
> > +}
> > +#endif
> > +
> > +void InbandGenericTextTrack::fileFailedToParse()
> > +{
> > +    LOG(Media, "Error parsing WebVTT stream.");
> > +}
> > +
> 
> This looks like code duplicated from InbandWebVTTTextTrack.cpp.  Is there any way to unify those two implementations?
> 

I don't have a good way to combine them, and I hope I can remove this completely in the future so I have left it for now.

> > Source/WebCore/platform/graphics/ISOVTTCue.cpp:145
> > +        else
> > +            LOG(Media, "ISOWebVTTCue::ISOWebVTTCue - skipping box id = \"%s\", size = %zu", boxType.utf8().data(), boxSize);
> 
> Should this be an ASSERT_UNREACHED, or do we expect unexpected box types?
> 

Unexpected box types are not illegal.

> > Source/WebCore/platform/graphics/ISOVTTCue.h:38
> > +// A ISOBox represents a ISO-BMFF box. Data in the structure is big-endian. The layout of the data structure as follows:
> 
> nit: "An ISOBox"

Fixed.

Thanks!

[Eric Carlson]

Committed http://trac.webkit.org/changeset/170323, with a followup fix in https://trac.webkit.org/r170324

[WebKit Commit Bot]

Re-opened since this is blocked by bug 134224

[Eric Carlson]

Created attachment 233676 [details]
Patch for the Windows bots to try.

[WebKit Commit Bot]

Attachment 233676 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ISOVTTCue.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 233676 [details]
Patch for the Windows bots to try.

Attachment 233676 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4790173812391936

New failing tests:
fullscreen/video-cursor-auto-hide.html

[Build Bot]

Created attachment 233682 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Eric Carlson]

Created attachment 233697 [details]
One more patch

[Eric Carlson]

Committed r170379: https://trac.webkit.org/r170379

[Eric Carlson]

The latest patch builds on Windows for Brent, so our theory is that the EWS build failure it spurious but I will watch the builders be certain

[Eric Carlson]

Plus a build fix in https://trac.webkit.org/r170384