Bug 103663 – [Mac] Add support for in-band text tracks

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Initial support for in-band text tracks on OS X.

[Radar WebKit Bug Importer]

<rdar://problem/12779668>

[Eric Carlson]

Created attachment 177020 [details]
Proposed patch

[Eric Carlson]

Created attachment 177028 [details]
Rebased patch

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 177028 [details]
Rebased patch

Attachment 177028 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15061509

[Jer Noble]

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.

[Eric Carlson]

Created attachment 177052 [details]
Updated patch

[Eric Carlson]

(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.

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 177052 [details]
Updated patch

Attachment 177052 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15063525

[Eric Carlson]

Created attachment 177065 [details]
One more update

[Silvia Pfeiffer]

For Chromium, feel free to point to bug 102708 in the test expectations.

Looks good to me.

[Sam Weinig]

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.

[Eric Carlson]

Created attachment 177543 [details]
Another updated patch.

[WebKit Review Bot]

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.

[Eric Carlson]

Created attachment 177547 [details]
Patch to fix style issues.

[Eric Carlson]

(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.

[WebKit Review Bot]

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.

[Eric Carlson]

Created attachment 177560 [details]
Fix the newly detected style problems.

[Build Bot]

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

[Eric Carlson]

Created attachment 177892 [details]
Updated patch

[Eric Carlson]

Created attachment 178017 [details]
Rebased patch.

Another day, another rebase.

[Sam Weinig]

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.

[Silvia Pfeiffer]

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?

[Eric Carlson]

Created attachment 178096 [details]
Another updated.

[Eric Carlson]

(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.

[Eric Carlson]

Created attachment 178101 [details]
Rebased again.

[Sam Weinig]

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.

[Eric Carlson]

(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.

[Eric Carlson]

http://trac.webkit.org/changeset/137161

[Ryosuke Niwa]

*** Bug 105514 has been marked as a duplicate of this bug. ***