[Proposal] types: simplify types for PlaybackObserver by Florent-Bouisset · Pull Request #1589 · canalplus/rx-player

Florent-Bouisset · 2024-10-31T16:26:25Z

Part of the code relies on the PlaybackObserver and redefines the properties needed in the PlaybackObservation it's listening to.

The types

IRepresentationEstimatorPlaybackObservation
IWorkerPlaybackObservation
ICmcdDataBuilderPlaybackObservation

Are all subsets of ICorePlaybackObservation, I propose to remove those types and replace them with ICorePlaybackObservation

Similarly I removed the type IRepresentationStreamPlaybackObservation to only use IAdaptationStreamPlaybackObservation

peaBerberian · 2024-10-31T16:38:26Z

As the AdaptationStream is the one that has a dependency on the RepresentationStream, shouldn't the type be coming from the RepresentationStream to the AdaptationStream and not the reverse?

peaBerberian · 2024-10-31T17:14:30Z

src/playback_observer/worker_playback_observer.ts

@@ -1,76 +1,21 @@
+import type { ICorePlaybackObservation } from "../main_thread/init/utils/create_core_playback_observer";


This dependency also looks kind of wrong to me.
The PlaybackObserver shouldn't be dependent on main_thread to me, the reverse should.

peaBerberian · 2024-11-04T14:05:48Z

src/core/adaptive/adaptive_representation_selector.ts

 import type { CancellationSignal } from "../../utils/task_canceller";
 import TaskCanceller from "../../utils/task_canceller";
 import type { IBufferType } from "../segment_sinks";
+import type { IAdaptationStreamPlaybackObservation } from "../stream/adaptation";


Isn't it weird that the ABR rely on a type defined inside another module it does not care about?

Maybe we should put that type in a more neutral place

peaBerberian · 2024-11-04T14:06:53Z

src/core/cmcd/cmcd_data_builder.ts

-  ObservationPosition,
-} from "../../playback_observer";
+import type { IReadOnlyPlaybackObserver } from "../../playback_observer";
+import type { ICorePlaybackObservation } from "../../playback_observer/types";


We could maybe just re-export it just from the playback_observer path

peaBerberian · 2024-11-04T14:08:12Z

src/core/stream/adaptation/types.ts

 import type { IRepresentationEstimator } from "../../adaptive";
 import type { SegmentQueueCreator } from "../../fetchers";
 import type { IBufferType, SegmentSink } from "../../segment_sinks";
+import type { IPeriodStreamPlaybackObservation } from "../period";


To me the PeriodStream depends on the AdaptationStream, not the other way around

The thing is that they don't overlap well, IAdaptationStreamPlaybackObservation has bufferGap attribute in addition of the other attributes that as IPeriodStreamPlaybackObservation.
But they both defines the buffered attribute in a different way.

I will just define them as both types with no extends nor Omit

We could also use different property names to explicit that difference

peaBerberian · 2024-11-04T14:08:48Z

src/core/stream/representation/types.ts

 import type { IReadOnlySharedReference } from "../../../utils/reference";
 import type { SegmentQueue } from "../../fetchers";
 import type { IBufferType, SegmentSink } from "../../segment_sinks";
+import type { IAdaptationStreamPlaybackObservation } from "../adaptation";


I wrote it as a comment but you may have not seen it:

As the AdaptationStream is the one that has a dependency on the RepresentationStream, shouldn't the type be coming from the RepresentationStream to the AdaptationStream and not the reverse?

IAdaptationStreamPlaybackObservation and IRepresentationStreamPlaybackObservation are the same so I deleted the Representation one.
Yeah name is a bit off because of that, should I rename it? Any idea of a name that would fit for both? Or I can duplicate it but I'm not fan of..

peaBerberian · 2024-11-04T14:09:05Z

src/core/stream/representation/utils/append_segment_to_buffer.ts

  SegmentSink,
 } from "../../../segment_sinks";
-import type { IRepresentationStreamPlaybackObservation } from "../types";
+import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";


peaBerberian · 2024-11-04T14:09:16Z

src/core/stream/representation/utils/get_buffer_status.ts

-  IRepresentationStreamPlaybackObservation,
-  IQueuedSegment,
-} from "../types";
+import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";


peaBerberian · 2024-11-04T14:09:26Z

src/core/stream/representation/utils/push_init_segment.ts

-  IRepresentationStreamPlaybackObservation,
-  IStreamEventAddedSegmentPayload,
-} from "../types";
+import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";


peaBerberian · 2024-11-04T14:09:34Z

src/core/stream/representation/utils/push_media_segment.ts

-  IRepresentationStreamPlaybackObservation,
-  IStreamEventAddedSegmentPayload,
-} from "../types";
+import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";


peaBerberian · 2024-11-04T14:09:59Z

src/main_thread/init/multi_thread_content_initializer.ts

 import sendMessage from "./send_message";
 import type { ITextDisplayerOptions } from "./types";
 import { ContentInitializer } from "./types";
+


Is that blank line wanted?

peaBerberian · 2024-11-12T11:08:29Z

src/playback_observer/types.ts

+   * loading (and thus paused) but with autoPlay enabled.
+   */
+  pending: boolean | undefined;
+}


To me the PlaybackObserver has no concept of what the AdaptationStream or RepresentationStream is.

For "core" I'm ok with.

This is mainly a naming issue for me.

Florent-Bouisset added 4 commits October 31, 2024 16:09

remove EstimatorPlaybackObservation

48bbe04

remove IWorkerPlaybackobservation

ab07a1b

remove ICmcdDataBuilderPlaybackObservation

e630bb5

remove IRepresentationStreamPlaybackObservation

6791b75

peaBerberian reviewed Oct 31, 2024

View reviewed changes

move ICorePlaybackObservation in src/playbackObserver

0e1208a

Florent-Bouisset force-pushed the refactor/simplify-observer branch from f60ae8c to 0e1208a Compare November 4, 2024 10:51

peaBerberian reviewed Nov 4, 2024

View reviewed changes

Florent-Bouisset added 2 commits November 5, 2024 11:26

code review

ebf2d46

fix typescript import

6c8ccdd

peaBerberian added this to the 4.3.0 milestone Nov 6, 2024

peaBerberian reviewed Nov 12, 2024

View reviewed changes

peaBerberian added code Priority: 2 (Medium) This issue or PR has a medium priority. labels Nov 22, 2024

peaBerberian removed the code label Mar 4, 2025

peaBerberian modified the milestones: 4.3.0, 4.4.0 Apr 7, 2025

peaBerberian force-pushed the dev branch 2 times, most recently from 00fc806 to b7216b4 Compare April 15, 2025 18:14

peaBerberian modified the milestones: 4.4.0, 4.5.0 Sep 19, 2025

peaBerberian force-pushed the dev branch 8 times, most recently from 6d4fed2 to 9b856a5 Compare September 25, 2025 15:56

peaBerberian force-pushed the dev branch 4 times, most recently from 6cfd206 to 1e55170 Compare October 13, 2025 20:54

peaBerberian force-pushed the dev branch from 8399707 to a42734c Compare December 10, 2025 15:07

github-actions bot force-pushed the dev branch from a42734c to 29372ac Compare December 10, 2025 15:50

peaBerberian force-pushed the dev branch 2 times, most recently from d4be192 to 9ad6758 Compare December 19, 2025 19:49

peaBerberian force-pushed the dev branch 9 times, most recently from 0142e34 to 1fd9df3 Compare January 27, 2026 11:59

		@@ -1,76 +1,21 @@
		import type { ICorePlaybackObservation } from "../main_thread/init/utils/create_core_playback_observer";

Conversation

Florent-Bouisset commented Oct 31, 2024

Uh oh!

peaBerberian commented Oct 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants