Fix a bug where `Media#extras` property is lost when handling updates to `playlist` (`MPV_EVENT_PROPERTY_CHANGE`)

[default-anton]

Steps to reproduce

// Create a playlist where each media has unique clip_id in extras
final playlist = Playlist([
  Media(
    'file.mp4',
    extras: {'clip_id': 'file.mp4 1'},
    start: Duration.zero,
    end: Duration(seconds: 10),
  ),
  Media(
    'file.mp4',
    extras: {'clip_id': 'file.mp4 2'},
    start: Duration(seconds: 10),
    end: Duration(seconds: 20),
  ),
]);

await player.open(playlist);

for (final media in player.state.playlist.medias) {
  debugPrint('media: ${media.extras}');
}

// Prints:
// media: {clip_id: file.mp4 2}
// media: {clip_id: file.mp4 2}

Observe that every instance of Media has extras of the last media in the playlist.

[alexmercerind]

I don't fully understand the fix. Please add a unit-test for the situation that this pull-request fixes.

We have multiple unit-tests for the issue you are mentioning (where it doesn't occur): https://github.com/media-kit/media-kit/blob/77a130b1d7ce733b47d2133b57563716090450d0/media_kit/test/src/player/player_test.dart#L508 https://github.com/media-kit/media-kit/blob/77a130b1d7ce733b47d2133b57563716090450d0/media_kit/test/src/player/player_test.dart#L762 https://github.com/media-kit/media-kit/blob/77a130b1d7ce733b47d2133b57563716090450d0/media_kit/test/src/player/player_test.dart#L553 https://github.com/media-kit/media-kit/blob/77a130b1d7ce733b47d2133b57563716090450d0/media_kit/test/src/player/player_test.dart#L846

[default-anton]

When we handle the playlist event, we create a new playlist from the state of the mpv player. https://github.com/media-kit/media-kit/blob/main/media_kit/lib/src/player/native/player/real.dart#L1688-L1713

This line initializes Media without start, end, and extras https://github.com/media-kit/media-kit/blob/main/media_kit/lib/src/player/native/player/real.dart#L1708

Later, we copy start and end to the newly initialized Media, but extras is left out. https://github.com/media-kit/media-kit/blob/main/media_kit/lib/src/player/native/player/real.dart#L1717-L1726

I'll write a test to reproduce this problem.

[abdelaziz-mahdy]

pull from main here too to fix ci

[default-anton]

After a little bit of digging, I finally found the root cause. I've added a test case for this edge case.

Media.cache uses uri as a cache key. In my case, I need to play the same uri with different start and end timestamps.

https://github.com/media-kit/media-kit/blob/2c4cbc54500f5b3e3810500b7762b801d2f534d5/media_kit/lib/src/models/media/media_native.dart#L110-L113

If you look closely at my example, you will see that two Media clips use file.mp4 but start and end are different. https://github.com/media-kit/media-kit/pull/779#issue-2254905716

All tests are green because we use unique video files https://github.com/media-kit/media-kit/blob/2c4cbc54500f5b3e3810500b7762b801d2f534d5/media_kit/test/common/sources_native.dart#L13-L18

The Question Is

Should we use '$uri:$start:$end' as cache key?

[alexmercerind]

I see. There's so much implementation that I have myself forgotten the way I made the things.

Sorry, it's not good to accept this pull request then. The things are ideal the way they are, including current GC finalizer mechanism.

Regarding macOS unit-tests, there are many flaky tests (infact many flaky ones are disabled presently too).

[alexmercerind]

Alternative proposal can be to complete remove this cache mechanism & handle extras, http headers like start & end (within Player instance).

[default-anton]

Alternative proposal can be to complete remove this cache mechanism & handle extras, http headers like start & end (within Player instance).

I like this proposal. Ok If I make this change?

Regarding macOS unit-tests, there are many flaky tests (infact many flaky ones are disabled presently too).

I can temporarily disable the flaky tests on macOS. I can debug them later.

[abdelaziz-mahdy]

Alternative proposal can be to complete remove this cache mechanism & handle extras, http headers like start & end (within Player instance).

I think that will help with custom backends? I am not sure but if it will that will be awesome

[alexmercerind]

Alternative proposal can be to complete remove this cache mechanism & handle extras, http headers like start & end (within Player instance).

I like this proposal. Ok If I make this change?

Sounds good to me. Removing the GC finalizer based logic entirely will be good way forward.