5G-MAG / rt-5gms-media-session-handler

5G Media Streaming - Media Session Handler
https://www.5g-mag.com/streaming
Other
4 stars 4 forks source link

5GMS Consumption reporting: Add support to Media Session Handler #38

Closed shilinding closed 11 months ago

shilinding commented 1 year ago

Get consumption report from Media Stream Handler and post to AF via M5.

Resolves #3.

dsilhavy commented 1 year ago

Another general comment: The Media Session Handler maintains a timer to request consumption reports from the Media Stream Handler. This seems to be missing here?

shilinding commented 12 months ago

Another general comment: The Media Session Handler maintains a timer to request consumption reports from the Media Stream Handler. This seems to be missing here?

The current implementation is: Media Stream Handler maintains a timer to report consumption reports to Media Session Handler, the timer is in Media Stream Handler, is it OK?

dsilhavy commented 12 months ago

Another general comment: The Media Session Handler maintains a timer to request consumption reports from the Media Stream Handler. This seems to be missing here?

The current implementation is: Media Stream Handler maintains a timer to report consumption reports to Media Session Handler, the timer is in Media Stream Handler, is it OK?

In my opinion, the Media Session Handler should maintain the timer according to reportingInterval. This would match with what we are doing for metrics reporting. If the reportingInterval changes during a streaming session the Media Session Handler can act accordingly and adjust its timer.

However, your approach might also be valid if I look at 26.501 5.6.1. But you need to make sure that you report according to reportingInterval.It looks like this is still a Todo here: https://github.com/5G-MAG/rt-5gms-media-stream-handler/pull/49/files

We can discuss the best approach tomorrow in the call. Maybe @rjb1000 also has some thoughts on this.

rjb1000 commented 12 months ago

In my opinion, the Media Session Handler should maintain the timer according to reportingInterval. This would match with what we are doing for metrics reporting. If the reportingInterval changes during a streaming session the Media Session Handler can act accordingly and adjust its timer.

However, your approach might also be valid if I look at 26.501 5.6.1. But you need to make sure that you report according to reportingInterval. It looks like this is still a Todo here: https://github.com/5G-MAG/rt-5gms-media-stream-handler/pull/49/files

We can discuss the best approach tomorrow in the call. Maybe @rjb1000 also has some thoughts on this.

TS 26.501 probably doesn't specify whether the reporting interval timer should be in the Media Session Handler or the Media Stream Handler. What is important is that the Media Session Handler regularly checks for changes to the client metrics reporting configuration by re-requesting the Service Access Information and that the 5GMS Client takes account of any changes in reporting frequency.

I couldn't find many clues in TS 26.512 clauses 12 and 13.

Clause 13.2.4 defines a metricsConfiguration object exposed by the Media Session Handler (Media Player) at reference point M8. But there is no equivalent object for the consumption reporting configuration. This looks like a specification gap. Please raise a Standards issue to record this.

Table 13.2.5-1 defines some metrics-related notification events sent by the Media Stream Handler (Media Player) to the Media Session Handler at reference point M7:

  • _METRICADDED: Triggered every time a new metric is added.
  • _METRICCHANGED: The minimum bit rate that the ABR algorithms will choose. Use NaN for no limit.
  • _METRICUPDATED: Set to true if you would like DASH Client to keep downloading fragments in the background when the video element is paused.
  • _METRICSCHANGED: Triggered whenever there is a change to the overall metrics.

There are no notifications defined for consumption reporting. Again, this could be a specification gap that needs reporting as a Standards issue.

Given these specification gaps, it seems that this is all up to implementation choice. That said, it would make sense for the consumption reporting feature and the QoE metrics reporting feature to follow a common design pattern.

The intervals at which metrics/consumption reports are sent by the Media Session Handler to the 5GMS AF feels like a concern for the Media Session Handler alone. It feels more natural for the Media Session Handler to maintain the reporting timers and to decide when to send metrics/consumption reports. (So I think I agree with @dsilhavy.) These reports would contain all information logged since the last report was successfully submitted to the 5GMS AF. (The Media Session Handler needs to handle error cases at M5 and buffer information while it retries the report submission.)

Does that make sense?

(Sorry for the long post. It should probably be turned into a discussion...)

dsilhavy commented 11 months ago

@shilinding : I will wait for your final changes regarding the metrics reporting timer before providing another code review. Please let me know once you are done. Moreover, once the remaining changes in the Media Stream Handler are done we can run an end to end test.

shilinding commented 11 months ago

@shilinding : I will wait for your final changes regarding the metrics reporting timer before providing another code review. Please let me know once you are done. Moreover, once the remaining changes in the Media Stream Handler are done we can run an end to end test.

@dsilhavy I've push the changes including "move the timer for Consumption Report to MSH" and "move PlayerStates updating trigger to MSH", thanks.

dsilhavy commented 11 months ago

@shilinding Your changes are a good starting point. I noticed that there are still quite a few hardcoded values in the Media Stream Handler and the Media Session Handler. Moreover, at least one change in this PR breaks existing functionality not related to consumption reporting.

As I don't have access to the underlying branch for this PR I will merge your changes to development and then work myself on fixing the remaining issues in a new branch.

shilinding commented 11 months ago

@shilinding Your changes are a good starting point. I noticed that there are still quite a few hardcoded values in the Media Stream Handler and the Media Session Handler. Moreover, at least one change in this PR breaks existing functionality not related to consumption reporting.

As I don't have access to the underlying branch for this PR I will merge your changes to development and then work myself on fixing the remaining issues in a new branch.

I've applied for access to my forks for you and waiting for the Opensource team's approval. If done I'll let you know and won't add any more changes at this point untill you done. Thanks.