Summary: | [Mac] Add support for in-band text tracks | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, dino, feature-media-reviews, graouts, gyuyoung.kim, ojan.autocc, ojan, pnormand, rakuco, silviapf, silviapfeiffer1, webkit-bug-importer, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Eric Carlson
2012-11-29 13:09:53 PST
Created attachment 177020 [details]
Proposed patch
Created attachment 177028 [details]
Rebased patch
Attachment 177028 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:235: Path does not exist. [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:1204: unrecognized bug identifier "Bug(eric.carlson)" [test/expectations] [5]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:37: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:851: Declaration has space between type name and * in InbandTextTrackPrivateAVF *trackToEnable [whitespace/declaration] [3]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:856: Declaration has space between type name and * in InbandTextTrackPrivateAVF *track [whitespace/declaration] [3]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:880: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:881: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:882: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:883: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:884: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:893: Missing space after , [whitespace/comma] [3]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:929: One line control clauses should not use braces. [whitespace/braces] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1017: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1056: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1058: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1059: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 16 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 177028 [details] Rebased patch Attachment 177028 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15061509 Comment on attachment 177028 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=177028&action=review > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:856 > + for (unsigned i = 0; i < m_textTracks.size(); ++i) { > + InbandTextTrackPrivateAVF *track = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); Are these tracks guaranteed to be of InbandTextTrackPrivateAVF type? If so, would it be possible for m_textTracks to be that type instead of the generic one? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1240 > +InbandTextTrackPrivateAVFObjC::InbandTextTrackPrivateAVFObjC(MediaPlayerPrivateAVFoundationObjC* player, AVMediaSelectionOption* selection, int trackIndex) > + : InbandTextTrackPrivateAVF(player, trackIndex) > + , m_mediaSelectionOption(selection) > +{ > +} We're starting to have a proliferation of classes inside this file. It might be worth considering splitting some of them into their own implementation files where possible. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1257 > + if ([mediaType isEqualToString:AVMediaTypeClosedCaption]) > + return captions; > + if ([mediaType isEqualToString:AVMediaTypeSubtitle]) > + return subtitles; While they're not part of this patch, the values for this enum should be capitalized. Same for InbandTextTrackPrivate::Mode. > LayoutTests/ChangeLog:1376 > + Extra space here. Created attachment 177052 [details]
Updated patch
(In reply to comment #6) > (From update of attachment 177028 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177028&action=review > > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:856 > > + for (unsigned i = 0; i < m_textTracks.size(); ++i) { > > + InbandTextTrackPrivateAVF *track = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); > > Are these tracks guaranteed to be of InbandTextTrackPrivateAVF type? If so, would it be possible for m_textTracks to be that type instead of the generic one? > That doesn't work because MediaPlayerPrivateAVFoundationObjC::getTextTracks() takes a “Vector<RefPtr<InbandTextTrackPrivate> >&. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1240 > > +InbandTextTrackPrivateAVFObjC::InbandTextTrackPrivateAVFObjC(MediaPlayerPrivateAVFoundationObjC* player, AVMediaSelectionOption* selection, int trackIndex) > > + : InbandTextTrackPrivateAVF(player, trackIndex) > > + , m_mediaSelectionOption(selection) > > +{ > > +} > > We're starting to have a proliferation of classes inside this file. It might be worth considering splitting some of them into their own implementation files where possible. > Yeah, there is more to do here so I will clean it up in a later patch. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1257 > > + if ([mediaType isEqualToString:AVMediaTypeClosedCaption]) > > + return captions; > > + if ([mediaType isEqualToString:AVMediaTypeSubtitle]) > > + return subtitles; > > While they're not part of this patch, the values for this enum should be capitalized. Same for InbandTextTrackPrivate::Mode. > Done. > > LayoutTests/ChangeLog:1376 > > + > > Extra space here. Done. Attachment 177052 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
LayoutTests/platform/mac/TestExpectations:1204: unrecognized bug identifier "Bug(eric.carlson)" [test/expectations] [5]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 177052 [details] Updated patch Attachment 177052 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15063525 Created attachment 177065 [details]
One more update
For Chromium, feel free to point to bug 102708 in the test expectations. Looks good to me. Comment on attachment 177065 [details] One more update View in context: https://bugs.webkit.org/attachment.cgi?id=177065&action=review > Source/WebCore/html/HTMLMediaElement.cpp:2756 > + RefPtr<TextTrack> textTrack = InbandTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this, privateTrack); privateTrack should probably have a .get() here, it is kind of weird to pass a RefPtr<> to a function taking a PassRefPtr (I wonder why we allow that). > Source/WebCore/html/HTMLMediaElement.h:673 > + Vector<RefPtr<InbandTextTrackPrivate> > m_privateTextTracks; Seems like this should be a Vector of RefPtr<InbandTextTrack>s not RefPtr<InbandTextTrackPrivate>s. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:608 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 You should switch these to a more descriptive macro, eg. HAVE(AV_FOUNDATION_INBAND_TRACK_SUPPORT). You can probably make it more concise. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:861 > + InbandTextTrackPrivateAVF* trackToEnable = 0; > + > + // AVFoundation can only emit cues for one track at a time, so enable the first track that is showing, or the first that > + // is hidden if none are showing, otherwise disable all tracks. > + for (unsigned i = 0; i < m_textTracks.size(); ++i) { > + InbandTextTrackPrivateAVF* track = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); > + if (track->mode() == InbandTextTrackPrivate::showing) { We usually like to keep RefCounted objects in RefPtr<>s on the stack. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:892 > + String attributedStringValue = String(CFAttributedStringGetString(attributedString)); I don't think you need to call the String constructor explicitly here. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1011 > + content.append(tagStart); > + content.append(attributedStringValue.substring(effectiveRange.location, effectiveRange.length)); > + content.append(tagEnd); The fact that we are using strings to build this markup worries me, it seems ripe for an injection attack. Can we build up the DOM programmatically? I realize that doing it in this class would be a clear layering violation, as this is in platform/ but perhaps we can have our own intermediate representation that and the HTMLMediaElement layer can construct the DOM from that. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1066 > + m_currentCueId = emptyString(); Why empty string rather than null string, which is what the constructor would set it to? > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:306 > +class InbandTextTrackPrivateAVF : public InbandTextTrackPrivate { > +public: Ideally this class would have its own header/implementation files. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:315 > + void processCue(CFArrayRef, Float64); Can this use double rather than Float64? > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:316 > + void processCueAttributes(CFAttributedStringRef, StringBuilder& content, StringBuilder& settings); Does anyone outside of this class call this function? Can it be private? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:199 > +class InbandTextTrackPrivateAVFObjC : public InbandTextTrackPrivateAVF { > +public: Ideally this class would have tis own header/implementation file. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:881 > + m_legibleOutput.adoptNS([[AVPlayerItemLegibleOutput alloc] initWithMediaSubtypesForNativeRepresentation:nil]); The preferred idiom is now m_legibleOutput = adoptNS([...]); > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1199 > + for (AVMediaSelectionOption *option in [legibleGroup options]) { > + > + if ([[option mediaType] isEqualToString:AVMediaTypeSubtitle]) { Extra newline. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1268 > + return emptyString(); If you want to return the empty string here the emptyAtom is preferred as it is already an AtomicString. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1272 > + if ([titles count]) > + { { should go on the previous line. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1281 > + return emptyString(); If you want to return the empty string here the emptyAtom is preferred as it is already an AtomicString. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1287 > + if (!m_mediaSelectionOption) > + return emptyString(); If you want to return the empty string here the emptyAtom is preferred as it is already an AtomicString. Created attachment 177543 [details]
Another updated patch.
Attachment 177543 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
Source/WebCore/platform/graphics/MediaPlayer.cpp:1118: "InbandTextTrack.h" already included at Source/WebCore/platform/graphics/MediaPlayer.cpp:35 [build/include] [4]
Source/WebCore/html/track/InbandTextTrack.h:50: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/html/track/InbandTextTrack.h:50: The parameter name "client" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:53: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:57: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:56: Extra space before ) [whitespace/parens] [2]
Total errors found: 6 in 31 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 177547 [details]
Patch to fix style issues.
(In reply to comment #13) > (From update of attachment 177065 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177065&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:2756 > > + RefPtr<TextTrack> textTrack = InbandTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this, privateTrack); > > privateTrack should probably have a .get() here, it is kind of weird to pass a RefPtr<> to a function taking a PassRefPtr (I wonder why we allow that). > Done. > > Source/WebCore/html/HTMLMediaElement.h:673 > > + Vector<RefPtr<InbandTextTrackPrivate> > m_privateTextTracks; > > Seems like this should be a Vector of RefPtr<InbandTextTrack>s not RefPtr<InbandTextTrackPrivate>s. > Removed. The media engine is now responsible for calling when text tracks are added or removed. > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:608 > > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 > > You should switch these to a more descriptive macro, eg. HAVE(AV_FOUNDATION_INBAND_TRACK_SUPPORT). You can probably make it more concise. > Done, changed to HAVE(AVFOUNDATION_TEXT_TRACK_SUPPORT). > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:861 > > + InbandTextTrackPrivateAVF* trackToEnable = 0; > > + > > + // AVFoundation can only emit cues for one track at a time, so enable the first track that is showing, or the first that > > + // is hidden if none are showing, otherwise disable all tracks. > > + for (unsigned i = 0; i < m_textTracks.size(); ++i) { > > + InbandTextTrackPrivateAVF* track = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); > > + if (track->mode() == InbandTextTrackPrivate::showing) { > > We usually like to keep RefCounted objects in RefPtr<>s on the stack. > Done. > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:892 > > + String attributedStringValue = String(CFAttributedStringGetString(attributedString)); > > I don't think you need to call the String constructor explicitly here. > Done. > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1011 > > + content.append(tagStart); > > + content.append(attributedStringValue.substring(effectiveRange.location, effectiveRange.length)); > > + content.append(tagEnd); > > The fact that we are using strings to build this markup worries me, it seems ripe for an injection attack. Can we build up the DOM programmatically? I realize that doing it in this class would be a clear layering violation, as this is in platform/ but perhaps we can have our own intermediate representation that and the HTMLMediaElement layer can construct the DOM from that. > This function creates WebVTT settings and cue text which are consumed by the WebVTT parser, so if there is a potential injection attack it we have a more serious problem with WebVTT files proper. Never-the-less, I will look at doing this a different way in a follow up patch. > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:1066 > > + m_currentCueId = emptyString(); > > Why empty string rather than null string, which is what the constructor would set it to? > Done. > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:306 > > +class InbandTextTrackPrivateAVF : public InbandTextTrackPrivate { > > +public: > > Ideally this class would have its own header/implementation files. > Done. > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:315 > > + void processCue(CFArrayRef, Float64); > > Can this use double rather than Float64? > Done. > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:316 > > + void processCueAttributes(CFAttributedStringRef, StringBuilder& content, StringBuilder& settings); > > Does anyone outside of this class call this function? Can it be private? > Done. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:199 > > +class InbandTextTrackPrivateAVFObjC : public InbandTextTrackPrivateAVF { > > +public: > > Ideally this class would have tis own header/implementation file. > Done. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:881 > > + m_legibleOutput.adoptNS([[AVPlayerItemLegibleOutput alloc] initWithMediaSubtypesForNativeRepresentation:nil]); > > The preferred idiom is now m_legibleOutput = adoptNS([...]); > Done. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1199 > > + for (AVMediaSelectionOption *option in [legibleGroup options]) { > > + > > + if ([[option mediaType] isEqualToString:AVMediaTypeSubtitle]) { > > Extra newline. > Removed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1268 > > + return emptyString(); > > If you want to return the empty string here the emptyAtom is preferred as it is already an AtomicString. > Done. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1272 > > + if ([titles count]) > > + { > > { should go on the previous line. > Done. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1281 > > + return emptyString(); > > If you want to return the empty string here the emptyAtom is preferred as it is already an AtomicString. > Done. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1287 > > + if (!m_mediaSelectionOption) > > + return emptyString(); > > If you want to return the empty string here the emptyAtom is preferred as it is already an AtomicString. > Done. Attachment 177547 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:53: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:57: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Total errors found: 2 in 31 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 177560 [details]
Fix the newly detected style problems.
Comment on attachment 177560 [details] Fix the newly detected style problems. Attachment 177560 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15132619 Created attachment 177892 [details]
Updated patch
Created attachment 178017 [details]
Rebased patch.
Another day, another rebase.
Comment on attachment 178017 [details] Rebased patch. View in context: https://bugs.webkit.org/attachment.cgi?id=178017&action=review > Source/WebCore/html/HTMLMediaElement.cpp:2733 > + RefPtr<TextTrack> textTrack = InbandTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this, prpTrack); The type of textTract should be RefPtr<InbandTextTrack>, there is no reason to use a less specific type here I don't think. > Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:40 > +class InbandTextTrackPrivate : public RefCounted<InbandTextTrackPrivate> { Not crazy about this name, but don't have something immediately better. > Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:67 > + void setTrack(TextTrack* track) { m_track = track; } > + TextTrack* textTrack() { return m_track; } This is a layering violation. TextTrack is in html/. All interaction with the TextTrack should probably be through the client. > Source/WebCore/platform/graphics/MediaPlayer.cpp:46 > #if ENABLE(VIDEO_TRACK) > +#include "InbandTextTrack.h" > #include "InbandTextTrackPrivate.h" > #endif This is a layering violation. platform/graphics cannot reference InbandTextTrack.h as it is in html/. You can probably fix this by moving InbandTextTrackClient to platform, as I think that is the only thing you are using this #include for. > Source/WebCore/platform/graphics/MediaPlayer.cpp:1123 > + m_mediaPlayerClient->mediaPlayerDidAddTrack(track.get()); No reason to call .get() > Source/WebCore/platform/graphics/MediaPlayer.cpp:1131 > + m_mediaPlayerClient->mediaPlayerDidRemoveTrack(track.get()); No reason to call .get(). > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:283 > +} > + > + > +} // namespace WebCore Extra newlines. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:36 > +#include "InbandTextTrack.h" Again, this a layering violation. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:122 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 Can we use a more specific macro? > Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObjC.h:39 > +#ifndef __OBJC__ > +typedef struct objc_object *id; > +#endif I don't see id being used in this header. > Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObjC.mm:39 > +#import "TextTrack.h" Including TextTrack here is a layering violation. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:43 > +#import "TextTrack.h" Layering violation. Some maybe helpful questions of clarification. > Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:56 > + enum Kind { subtitles, captions, descriptions, chapters, metadata, none }; Would it be possible to avoid defining Kind and Mode values in multiple classes? > > Source/WebCore/platform/graphics/MediaPlayer.cpp:46 > > #if ENABLE(VIDEO_TRACK) > > +#include "InbandTextTrack.h" > > #include "InbandTextTrackPrivate.h" > > #endif > > This is a layering violation. platform/graphics cannot reference InbandTextTrack.h as it is in html/. You can probably fix this by moving InbandTextTrackClient to platform, as I think that is the only thing you are using this #include for. Might it make sense to have an interface class of InbandTextTrack in html/ so all platforms implement them the same way but the platform dependent implementation goes to platform/graphics? Created attachment 178096 [details]
Another updated.
(In reply to comment #24) > Some maybe helpful questions of clarification. > > > Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:56 > > + enum Kind { subtitles, captions, descriptions, chapters, metadata, none }; > > Would it be possible to avoid defining Kind and Mode values in multiple classes? > We can't use the ones in /html from /platform/graphics. We could define the strings here and use them in TextTrack, but I don't think that make sense. > > > > Source/WebCore/platform/graphics/MediaPlayer.cpp:46 > > > #if ENABLE(VIDEO_TRACK) > > > +#include "InbandTextTrack.h" > > > #include "InbandTextTrackPrivate.h" > > > #endif > > > > This is a layering violation. platform/graphics cannot reference InbandTextTrack.h as it is in html/. You can probably fix this by moving InbandTextTrackClient to platform, as I think that is the only thing you are using this #include for. > > Might it make sense to have an interface class of InbandTextTrack in html/ so all platforms implement them the same way but the platform dependent implementation goes to platform/graphics? That is what we have. InbandTextTrack.h is in html/, but media engine code in /platform/graphics/ can't access that so we have InbandTextTrackPrivate.h in platform/graphics/. This is the interface that all platform media engines implements. Created attachment 178101 [details]
Rebased again.
Comment on attachment 178101 [details] Rebased again. View in context: https://bugs.webkit.org/attachment.cgi?id=178101&action=review > Source/WebCore/platform/graphics/MediaPlayer.h:65 > -class TextTrackClient; > +class ScriptExecutionContext; I don't understand this change. It is also a layering violation. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:827 > + RefPtr<InbandTextTrackPrivateAVF>(track) = m_textTracks[i]; That's a bit wacky. I am surprised that does anything. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:68 > + void processCue(NSArray *, Float64); Is Float64 different than double? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1226 > + RefPtr<InbandTextTrackPrivateAVF>(track) = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); Weird syntax again. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1237 > + RefPtr<InbandTextTrackPrivateAVF>(track) = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); Weird syntax again. (In reply to comment #28) > (From update of attachment 178101 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178101&action=review > > > Source/WebCore/platform/graphics/MediaPlayer.h:65 > > -class TextTrackClient; > > +class ScriptExecutionContext; > > I don't understand this change. It is also a layering violation. > Oops - left over cruft from an earlier, abandoned, approach. Removed > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:827 > > + RefPtr<InbandTextTrackPrivateAVF>(track) = m_textTracks[i]; > > That's a bit wacky. I am surprised that does anything. > Yeah... Fixed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:68 > > + void processCue(NSArray *, Float64); > > Is Float64 different than double? > Only in the way it is spelled. Fixed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1226 > > + RefPtr<InbandTextTrackPrivateAVF>(track) = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); > > Weird syntax again. > Fixed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1237 > > + RefPtr<InbandTextTrackPrivateAVF>(track) = static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get()); > > Weird syntax again. > Fixed. *** Bug 105514 has been marked as a duplicate of this bug. *** |