Bug 232194 - Likely typo in MediaPlayerPrivateAVFoundationObjC::updateVideoTracks()
Summary: Likely typo in MediaPlayerPrivateAVFoundationObjC::updateVideoTracks()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-22 20:55 PDT by Jean-Yves Avenard [:jya]
Modified: 2021-10-24 17:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.72 KB, patch)
2021-10-22 21:04 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (3.26 KB, patch)
2021-10-24 16:05 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2021-10-22 20:55:00 PDT
In MediaPlayerPrivateAVFoundationObjC::updateVideoTracks() we see:

```
    for (auto& track : m_audioTracks)
        track->resetPropertiesFromTrack();

```

it's almost certainly wrong and should be:
```
    for (auto& track : m_videoTracks)
        track->resetPropertiesFromTrack();
```
Comment 1 Radar WebKit Bug Importer 2021-10-22 20:55:19 PDT
<rdar://problem/84574134>
Comment 2 Jean-Yves Avenard [:jya] 2021-10-22 21:04:15 PDT
Created attachment 442247 [details]
Patch
Comment 3 Darin Adler 2021-10-23 09:16:59 PDT
Comment on attachment 442247 [details]
Patch

Typically when we spot an obvious mistake like this, we spend a moment to try to create a test that failed because of the error. It’s frustrating, because likely if we had written the code correctly we would not have to think about how to write a test.

Please consider if we could have tested. I have personally been in this situation many times, spotted a typo that’s an obvious mistake and then was frustrated at the challenge of writing a test to prove the code was malfunctioning, and I understand if after thinking it over you do not find it’s practical to test, but I would appreciate a specific comment saying so if that happened.
Comment 4 Jean-Yves Avenard [:jya] 2021-10-23 20:53:54 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 442247 [details]
> Patch
> 
> Typically when we spot an obvious mistake like this, we spend a moment to
> try to create a test that failed because of the error. It’s frustrating,
> because likely if we had written the code correctly we would not have to
> think about how to write a test.

I found this while reading the code for a totally unrelated bug. 

I did try to understand the consequences of such error. This code was written in 2014, so the severity of the problem is likely low. 

To hit that code, you would need to write a specially crafted HLS stream, which in itself isn’t trivial as playback of those is non deterministic. And even so, it would likely not be observable via JS. 

I will add a comment indicating so.
Comment 5 Jean-Yves Avenard [:jya] 2021-10-24 16:05:16 PDT
Created attachment 442327 [details]
Patch
Comment 6 EWS 2021-10-24 17:04:19 PDT
Committed r284770 (243479@main): <https://commits.webkit.org/243479@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442327 [details].