Bug 137474 – [Mac] Represent AVMediaSelectionOptions as AudioTracks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[Mac] Represent AVMediaSelectionOptions as AudioTracks

[Jer Noble]

Created attachment 239389 [details]
Patch

[Jer Noble]

Created attachment 239413 [details]
Patch

[Brent Fulgham]

Comment on attachment 239413 [details]
Patch

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

I think this needs some cleanup before landing, but otherwise looks great!

> Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.h:78
> +    RefPtr<MediaSelectionOptionAVFObjC> m_mediaSelectionOption;

It almost seems like these should be two distinct classes; AVTrackPrivates that are backed by AVPlayerItemTracks, and AVTrackPrivates that are backed by media selection options. But that would probably add needless complexity...

The upside would be that it would do away with the weird "dual" nature of all the methods below.

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:122
> +

Extra space...

> Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.h:58
> +    static RefPtr<AudioTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option)

Should this be a const reference?

> Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.mm:42
> +    : m_impl(std::make_unique<AVTrackPrivateAVFObjCImpl>(option))

Const reference for option?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1860
> +    for (auto &oldItem : oldItems) {

What!?! Ugh! Is this some horrible ObjC coding standard we use? Hmm. Elsewhere in the file we use "for (auto& oldItem..." like human beings, so I think this is a typo.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1876
> +    for (auto & addedItem : addedItems)

Zounds! Another variation on weird reference spacing. No!

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1916
> +    for (auto &oldItem : oldItems) {

Nit: Bad reference spacing.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1932
> +    for (auto & addedItem : addedItems)

Nit: Extra space here. On the LEFT side of the '&'.

> Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:47
> +VideoTrackPrivateAVFObjC::VideoTrackPrivateAVFObjC(MediaSelectionOptionAVFObjC& option)

Should this be a const reference, since we're using it as an argument to a constructor?

> Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:88
> +void VideoTrackPrivateAVFObjC::setMediaSelectonOption(MediaSelectionOptionAVFObjC& option)

Ditto: Const reference perhaps?

> Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.h:57
> +    static RefPtr<VideoTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option)

Nit: Maybe this should be a const reference since we're only using it to construct an object?

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 239413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239413&action=review
> 
> I think this needs some cleanup before landing, but otherwise looks great!
> 
> > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.h:78
> > +    RefPtr<MediaSelectionOptionAVFObjC> m_mediaSelectionOption;
> 
> It almost seems like these should be two distinct classes; AVTrackPrivates that are backed by AVPlayerItemTracks, and AVTrackPrivates that are backed by media selection options. But that would probably add needless complexity...
> 
> The upside would be that it would do away with the weird "dual" nature of all the methods below.

Yes, that's true, but there are a couple of places where they share part of an implementation. Language selection, for instance. And bifurcating them into two classes would require making this class virtual, and I wanted to avoid that if at all possible.

> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:122
> > +
> 
> Extra space...
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.h:58
> > +    static RefPtr<AudioTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option)
> 
> Should this be a const reference?

No, because the AVTrackPrivateAVFObjCImpl needs to be able to call non-const methods on it.

> > Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.mm:42
> > +    : m_impl(std::make_unique<AVTrackPrivateAVFObjCImpl>(option))
> 
> Const reference for option?

Ditto.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1860
> > +    for (auto &oldItem : oldItems) {
> 
> What!?! Ugh! Is this some horrible ObjC coding standard we use? Hmm. Elsewhere in the file we use "for (auto& oldItem..." like human beings, so I think this is a typo.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1876
> > +    for (auto & addedItem : addedItems)
> 
> Zounds! Another variation on weird reference spacing. No!
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1916
> > +    for (auto &oldItem : oldItems) {
> 
> Nit: Bad reference spacing.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1932
> > +    for (auto & addedItem : addedItems)
> 
> Nit: Extra space here. On the LEFT side of the '&'.

Ick.  Fixed all these spacing issues.

> > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:47
> > +VideoTrackPrivateAVFObjC::VideoTrackPrivateAVFObjC(MediaSelectionOptionAVFObjC& option)
> 
> Should this be a const reference, since we're using it as an argument to a constructor?

Same as above. No, because we need to be able to call non-const methods on it.

> > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:88
> > +void VideoTrackPrivateAVFObjC::setMediaSelectonOption(MediaSelectionOptionAVFObjC& option)
> 
> Ditto: Const reference perhaps?

Ditto.

> > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.h:57
> > +    static RefPtr<VideoTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option)
> 
> Nit: Maybe this should be a const reference since we're only using it to construct an object?

Ditto.

[Jer Noble]

Created attachment 239585 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2736:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


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

[Jer Noble]

Created attachment 239588 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2736:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


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

[Jer Noble]

Created attachment 239600 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2735:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


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

[Jer Noble]

Created attachment 239631 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2736:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


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

[Jer Noble]

Created attachment 239641 [details]
Patch for landing

[Jer Noble]

Created attachment 239642 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2740:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


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

[WebKit Commit Bot]

Comment on attachment 239642 [details]
Patch for landing

Rejecting attachment 239642 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 239642, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/4992267878137856

[Jon Lee]

rdar://problem/17290111

[Jer Noble]

Committed r174823: <http://trac.webkit.org/changeset/174823>

[Simon Fraser (smfr)]

Two tests are still failing after this change:
https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r174866%20(7482)/results.html

[Jer Noble]

(In reply to comment #19)
> Two tests are still failing after this change:
> https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/
> r174866%20(7482)/results.html

It looks like, for both these tests, 'canplaythrough' events are firing earlier than they were previously. I.e., before the <video> in question has audio or video tracks. I'm still looking into this, but for the meantime, I'll check in these as expected failures.

[Jer Noble]

(In reply to comment #20)
> (In reply to comment #19)
> > Two tests are still failing after this change:
> > https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/
> > r174866%20(7482)/results.html
> 
> It looks like, for both these tests, 'canplaythrough' events are firing
> earlier than they were previously. I.e., before the <video> in question has
> audio or video tracks. I'm still looking into this, but for the meantime,
> I'll check in these as expected failures.

Filed <https://bugs.webkit.org/show_bug.cgi?id=137882>