[Proposal] types: simplify types for PlaybackObserver #1589

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

@@ -36,6 +33,7 @@ import SharedReference from "../../utils/reference";
 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 { IReadOnlySharedReference } from "../../../utils/reference";
 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

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

src/core/stream/representation/types.ts

@@ -20,6 +17,7 @@ import type { IRange } from "../../../utils/ranges";
 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

@@ -32,7 +32,7 @@ import type {
  IPushChunkInfos,
  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

@@ -58,6 +58,7 @@ import type { ITextDisplayer } from "../text_displayer";
 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] types: simplify types for PlaybackObserver #1589

[Proposal] types: simplify types for PlaybackObserver #1589

Florent-Bouisset commented Oct 31, 2024

peaBerberian commented Oct 31, 2024

peaBerberian Oct 31, 2024

peaBerberian Nov 4, 2024

peaBerberian Nov 4, 2024

peaBerberian Nov 4, 2024

Florent-Bouisset Nov 5, 2024

peaBerberian Nov 4, 2024

Florent-Bouisset Nov 5, 2024

peaBerberian Nov 4, 2024

peaBerberian Nov 4, 2024

peaBerberian Nov 4, 2024

peaBerberian Nov 4, 2024

peaBerberian Nov 4, 2024

peaBerberian Nov 12, 2024

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

[Proposal] types: simplify types for PlaybackObserver #1589

Are you sure you want to change the base?

[Proposal] types: simplify types for PlaybackObserver #1589

Conversation

Florent-Bouisset commented Oct 31, 2024

peaBerberian commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment