Configure AVFoundation to deliver raw WebVTT captions.
<rdar://problem/13237650>
Created attachment 233577 [details] Patch for the bots to chew on.
Created attachment 233578 [details] Rebased patch
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.
Created attachment 233608 [details] Updated patch.
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.
Created attachment 233611 [details] Another patch.
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.
Created attachment 233618 [details] Eric's Patch Tweaked for Windows Build
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"
(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!
Committed http://trac.webkit.org/changeset/170323, with a followup fix in https://trac.webkit.org/r170324
Re-opened since this is blocked by bug 134224
Created attachment 233676 [details] Patch for the Windows bots to try.
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.
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
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
Created attachment 233697 [details] One more patch
Committed r170379: https://trac.webkit.org/r170379
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
Plus a build fix in https://trac.webkit.org/r170384