Bug 140398 – [Mac] HLS audio is not correctly selected according to system language

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

[Jer Noble]

[Mac] HLS audio is not correctly selected according to system language

[Jer Noble]

Created attachment 244529 [details]
Patch

[Darin Adler]

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.

[Jon Lee]

rdar://problem/19218487

[Jer Noble]

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

[Jer Noble]

Created attachment 245353 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 245353 [details]
Patch for landing

Clearing flags on attachment: 245353

Committed r179549: <http://trac.webkit.org/changeset/179549>

[Charlie Turner]

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?