Bug 140398 - [Mac] HLS audio is not correctly selected according to system language
Summary: [Mac] HLS audio is not correctly selected according to system language
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:
Blocks:
 
Reported: 2015-01-13 11:41 PST by Jer Noble
Modified: 2019-10-22 14:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.67 KB, patch)
2015-01-13 11:45 PST, Jer Noble
darin: review+
Details | Formatted Diff | Diff
Patch for landing (10.84 KB, patch)
2015-01-26 09:16 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2015-01-13 11:41:00 PST
[Mac] HLS audio is not correctly selected according to system language
Comment 1 Jer Noble 2015-01-13 11:45:57 PST
Created attachment 244529 [details]
Patch
Comment 2 Darin Adler 2015-01-14 15:43:06 PST
Comment on attachment 244529 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.h:94
> +    bool m_shouldSelectOptionAutomatically;

In new code, it’s better to initialize here:

    bool m_shouldSelectOptionAutomatically { true };

I suggest doing the same with m_selectedOption.

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:95
> +    , m_shouldSelectOptionAutomatically(true)

Then you would not have to initialize here.

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:136
> +    if (!m_shouldSelectOptionAutomatically)
> +        return;

Should we clear this flag after doing it once? Do we want to do this repeatedly on successive calls to updateOptions?

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:140
> +    RetainPtr<NSMutableArray> nsLanguages = adoptNS([[NSMutableArray alloc] initWithCapacity:userPreferredLanguages().size()]);
> +    for (auto& language : userPreferredLanguages())
> +        [nsLanguages addObject:(NSString*)language];

We ought to have a helper function that turns a Vector<String> into an NSArray containing NSString. This idiom comes up too often to have it written out like this.

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:141
> +    RetainPtr<NSArray> filteredOptions = [getAVMediaSelectionGroupClass() mediaSelectionOptionsFromArray:[m_mediaSelectionGroup options] filteredAndSortedAccordingToPreferredLanguages:nsLanguages.get()];

Why a RetainPtr here?

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:146
> +    AVMediaSelectionOption* preferredOption = [filteredOptions firstObject];

Looks like this does not compile on older versions of OS X. I’m guessing this has something to do with RetainPtr. Seems like we should have firstObject available since it was added in OS X 10.6.

> Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:154
> +    if (m_selectionTimer.isActive())
> +        m_selectionTimer.stop();
> +    m_selectionTimer.startOneShot(0);

I think you can leave out all that isActive/stop stuff; that’s part of what startOneShot guarantees anyway.
Comment 3 Jon Lee 2015-01-16 08:48:20 PST
rdar://problem/19218487
Comment 4 Jer Noble 2015-01-26 08:25:03 PST
(In reply to comment #2)
> Comment on attachment 244529 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244529&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.h:94
> > +    bool m_shouldSelectOptionAutomatically;
> 
> In new code, it’s better to initialize here:
> 
>     bool m_shouldSelectOptionAutomatically { true };
> 
> I suggest doing the same with m_selectedOption.

Neat. I was unaware of this C++11 feature.

> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:95
> > +    , m_shouldSelectOptionAutomatically(true)
> 
> Then you would not have to initialize here.
> 
> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:136
> > +    if (!m_shouldSelectOptionAutomatically)
> > +        return;
> 
> Should we clear this flag after doing it once? Do we want to do this
> repeatedly on successive calls to updateOptions?

Yes, we may need to allow the automatic selection to occur if new tracks appear, as will happen with HLS streams.

> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:140
> > +    RetainPtr<NSMutableArray> nsLanguages = adoptNS([[NSMutableArray alloc] initWithCapacity:userPreferredLanguages().size()]);
> > +    for (auto& language : userPreferredLanguages())
> > +        [nsLanguages addObject:(NSString*)language];
> 
> We ought to have a helper function that turns a Vector<String> into an
> NSArray containing NSString. This idiom comes up too often to have it
> written out like this.

True.

> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:141
> > +    RetainPtr<NSArray> filteredOptions = [getAVMediaSelectionGroupClass() mediaSelectionOptionsFromArray:[m_mediaSelectionGroup options] filteredAndSortedAccordingToPreferredLanguages:nsLanguages.get()];
> 
> Why a RetainPtr here?

I always wrap ObjC objects in RetainPtr<>, even if they're autoreleased.  Perhaps that's over-cautious.

> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:146
> > +    AVMediaSelectionOption* preferredOption = [filteredOptions firstObject];
> 
> Looks like this does not compile on older versions of OS X. I’m guessing
> this has something to do with RetainPtr. Seems like we should have
> firstObject available since it was added in OS X 10.6.

Aha! It existed in 10.6, but was not public. It was only made public in 10.9, and the NS_AVAILABLE() macro reflects when it existed, not when it was made public. I'll use a more standard idiom here.

> > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:154
> > +    if (m_selectionTimer.isActive())
> > +        m_selectionTimer.stop();
> > +    m_selectionTimer.startOneShot(0);
> 
> I think you can leave out all that isActive/stop stuff; that’s part of what
> startOneShot guarantees anyway.

Ok.
Comment 5 Jer Noble 2015-01-26 09:16:49 PST
Created attachment 245353 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2015-02-03 09:28:01 PST
Comment on attachment 245353 [details]
Patch for landing

Clearing flags on attachment: 245353

Committed r179549: <http://trac.webkit.org/changeset/179549>
Comment 7 Charlie Turner 2019-10-22 14:12:37 PDT
Am I right to think this is Mac specific behaviour? I don't think it's spec'd anywhere that we have to do this. In GStreamer it's not going to happen without quite a bit of churn, so I'd rather skip it if it's optional for now. Jer, can you confirm?