Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Proposal] types: simplify types for PlaybackObserver #1589

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

[Proposal] types: simplify types for PlaybackObserver #1589

wants to merge 7 commits into from

Conversation

Florent-Bouisset
Copy link
Collaborator

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
Copy link
Collaborator

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
peaBerberian reviewed Oct 31, 2024
View reviewed changes
src/playback_observer/worker_playback_observer.ts Outdated
@@ -1,76 +1,21 @@
import type { ICorePlaybackObservation } from "../main_thread/init/utils/create_core_playback_observer";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Florent-Bouisset Florent-Bouisset force-pushed the refactor/simplify-observer branch from f60ae8c to 0e1208a Compare November 4, 2024 10:51
peaBerberian
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/adaptive/adaptive_representation_selector.ts Outdated
@@ -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";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/cmcd/cmcd_data_builder.ts Outdated
ObservationPosition,
} from "../../playback_observer";
import type { IReadOnlyPlaybackObserver } from "../../playback_observer";
import type { ICorePlaybackObservation } from "../../playback_observer/types";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

peaBerberian
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/stream/adaptation/types.ts Outdated
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";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also use different property names to explicit that difference

peaBerberian
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/stream/representation/types.ts Outdated
@@ -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";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/stream/representation/utils/append_segment_to_buffer.ts Outdated
@@ -32,7 +32,7 @@ import type {
IPushChunkInfos,
SegmentSink,
} from "../../../segment_sinks";
import type { IRepresentationStreamPlaybackObservation } from "../types";
import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

peaBerberian
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/stream/representation/utils/get_buffer_status.ts Outdated
IRepresentationStreamPlaybackObservation,
IQueuedSegment,
} from "../types";
import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

peaBerberian
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/stream/representation/utils/push_init_segment.ts Outdated
IRepresentationStreamPlaybackObservation,
IStreamEventAddedSegmentPayload,
} from "../types";
import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

peaBerberian
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/core/stream/representation/utils/push_media_segment.ts Outdated
IRepresentationStreamPlaybackObservation,
IStreamEventAddedSegmentPayload,
} from "../types";
import type { IAdaptationStreamPlaybackObservation } from "../../adaptation";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

peaBerberian
peaBerberian reviewed Nov 4, 2024
View reviewed changes
src/main_thread/init/multi_thread_content_initializer.ts Outdated
@@ -58,6 +58,7 @@ import type { ITextDisplayer } from "../text_displayer";
import sendMessage from "./send_message";
import type { ITextDisplayerOptions } from "./types";
import { ContentInitializer } from "./types";

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that blank line wanted?

@peaBerberian peaBerberian added this to the 4.3.0 milestone Nov 6, 2024
peaBerberian
peaBerberian reviewed Nov 12, 2024
View reviewed changes
src/playback_observer/types.ts
* loading (and thus paused) but with autoPlay enabled.
*/
pending: boolean | undefined;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@peaBerberian peaBerberian added code Relative to the RxPlayer's code. Might not change any RxPlayer behavior Priority: 2 (Medium) This issue or PR has a medium priority. labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peaBerberian peaBerberian peaBerberian left review comments

Assignees
No one assigned
Labels
code Relative to the RxPlayer's code. Might not change any RxPlayer behavior Priority: 2 (Medium) This issue or PR has a medium priority.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Florent-Bouisset @peaBerberian