Bug 137474 - [Mac] Represent AVMediaSelectionOptions as AudioTracks
Summary: [Mac] Represent AVMediaSelectionOptions as AudioTracks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on: 137472
Blocks: 137882
  Show dependency treegraph
 
Reported: 2014-10-06 17:48 PDT by Jer Noble
Modified: 2014-10-20 08:41 PDT (History)
9 users (show)

See Also:


Attachments
Patch (53.15 KB, patch)
2014-10-06 22:34 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (1.29 MB, patch)
2014-10-07 09:04 PDT, Jer Noble
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (1.29 MB, patch)
2014-10-09 17:32 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (1.29 MB, patch)
2014-10-09 18:15 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (1.29 MB, patch)
2014-10-09 22:09 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (1.29 MB, patch)
2014-10-10 09:43 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (53.17 KB, patch)
2014-10-10 12:30 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (1.29 MB, patch)
2014-10-10 12:31 PDT, Jer Noble
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-10-06 17:48:29 PDT
[Mac] Represent AVMediaSelectionOptions as AudioTracks
Comment 1 Jer Noble 2014-10-06 22:34:20 PDT
Created attachment 239389 [details]
Patch
Comment 2 Jer Noble 2014-10-07 09:04:44 PDT
Created attachment 239413 [details]
Patch
Comment 3 Brent Fulgham 2014-10-07 11:18:26 PDT
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?
Comment 4 Jer Noble 2014-10-07 11:39:24 PDT
(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.
Comment 5 Jer Noble 2014-10-09 17:32:47 PDT
Created attachment 239585 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2014-10-09 17:35:01 PDT
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.
Comment 7 Jer Noble 2014-10-09 18:15:06 PDT
Created attachment 239588 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2014-10-09 18:17:36 PDT
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.
Comment 9 Jer Noble 2014-10-09 22:09:44 PDT
Created attachment 239600 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2014-10-09 22:14:25 PDT
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.
Comment 11 Jer Noble 2014-10-10 09:43:56 PDT
Created attachment 239631 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2014-10-10 09:50:50 PDT
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.
Comment 13 Jer Noble 2014-10-10 12:30:23 PDT
Created attachment 239641 [details]
Patch for landing
Comment 14 Jer Noble 2014-10-10 12:31:38 PDT
Created attachment 239642 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2014-10-10 13:21:51 PDT
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.
Comment 16 WebKit Commit Bot 2014-10-10 15:28:40 PDT
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
Comment 17 Jon Lee 2014-10-16 14:47:41 PDT
rdar://problem/17290111
Comment 18 Jer Noble 2014-10-17 09:40:15 PDT
Committed r174823: <http://trac.webkit.org/changeset/174823>
Comment 19 Simon Fraser (smfr) 2014-10-19 19:05:33 PDT
Two tests are still failing after this change:
https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r174866%20(7482)/results.html
Comment 20 Jer Noble 2014-10-20 08:23:34 PDT
(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.
Comment 21 Jer Noble 2014-10-20 08:41:23 PDT
(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>