-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
5GMS Consumption reporting: Add support to Media Session Handler #38
Conversation
@@ -123,6 +139,15 @@ class MediaSessionHandlerMessengerService() : Service() { | |||
val resource = | |||
handleServiceAccessResponse(response, sendingUid, provisioningSessionId) | |||
|
|||
// initialize Retrofit for ConsumptionReporting | |||
currentServiceAccessInformation = resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are maintaining multiple concurrent sessions in the Media Session Handler. This means that multiple applications can be connected to the same Media Session Handler (see handling in handleServiceAccessResponse
). There is no need to define a variable currentServiceAccessInformation
, you can use resource
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are maintaining multiple concurrent sessions in the Media Session Handler. This means that multiple applications can be connected to the same Media Session Handler (see handling in
handleServiceAccessResponse
). There is no need to define a variablecurrentServiceAccessInformation
, you can useresource
directly.
Because in some Consumption reporting related methods such as IsConsumptionReportingActivated and NeedReportConsumption, clientConsumptionReportingConfiguration of ServiceAccessInformation is need. So I need to save it as a member of the class MediaSessionHandlerMessengerService. I can only use resource directly only in handleStartPlaybackByServiceListEntryMessage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what clientsSessionData
can be used for:
clientsSessionData[sendingUid]?.serviceAccessInformation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks. Done
if(currentServiceAccessInformation!!.clientConsumptionReportingConfiguration.serverAddresses.isNotEmpty()) | ||
{ | ||
val serverAddressesForConsumpReport: String = currentServiceAccessInformation!!.clientConsumptionReportingConfiguration.serverAddresses[0]; | ||
Log.i(TAG, ">>>shilin: clientConsumptionReportingConfiguration serverAddresses0: $serverAddressesForConsumpReport.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
private fun initializeRetrofitForConsumpReport(url: String) { | ||
val m5RetrofitConsump: Retrofit = Retrofit.Builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retrofitBuilder
is already initialized you can use
val retrofit = retrofitBuilder
.baseUrl(m5BaseUrl)
.build()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.baseUrl(url) | ||
//.addConverterFactory(GsonConverterFactory.create()) | ||
.build() | ||
consumptionReportingApi = m5RetrofitConsump.create(ConsumptionReportingApi::class.java) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be bound to a specific client is as there are multiple applications connected to the same Media Session Handler. As an example for the Service Access Information:
clientsSessionData[msg.sendingUid]?.serviceAccessInformationApi = retrofit.create(ServiceAccessInformationApi::class.java)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -144,6 +169,7 @@ class MediaSessionHandlerMessengerService() : Service() { | |||
} | |||
|
|||
override fun onFailure(call: Call<ServiceAccessInformation?>, t: Throwable) { | |||
Log.i(TAG, ">>>shilin: debug onFailure") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return false; | ||
} | ||
|
||
if(SamplePercentage_100.toUInt() == samplePercentage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this function to check if reporting should be activated or not according to samplePercentage
:
private fun shouldReportAccordingToSamplePercentage(samplePercentage: Float?): Boolean {
if (samplePercentage != null && samplePercentage <= 0) {
return false
}
if (samplePercentage == null || samplePercentage >= 100.0) {
return true
}
return utils.generateRandomFloat() < samplePercentage
}
|
||
// check clientConsumptionReportingConfiguration.locationReporting | ||
// check clientConsumptionReportingConfiguration.accessReporting | ||
if(currentServiceAccessInformation!!.clientConsumptionReportingConfiguration.locationReporting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is consumption reporting only send if locationReporting
or accessReporting
is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just follow clause 4.7.4, as below:
If the consumption reporting procedure is activated, the Media Session Handler shall submit a consumption report to the 5GMSd AF when any of the following conditions occur:
- Start of consumption of a downlink streaming session;
- Stop of consumption of a downlink streaming session;
- Upon determining the need to report ongoing 5GMS consumption at periodic intervals determined by the clientConsumptionReportingConfiguration.reportingInterval property.
- Upon determining a location change, if the clientConsumptionReportingConfiguration.locationReporting property is set to True.
- Upon determining an access network change (e.g. unicast to eMBMS, or vice versa), if the clientConsumptionReportingConfiguration.accessReporting property is set to True.
Did I misunderstand it?
private fun reportConsumption(msg: Message) { | ||
if (!NeedReportConsumption()) | ||
{ | ||
Log.i(TAG, "shilin>>>Not need ReportConsumption") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all shilin
from the logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
response: Response<ResponseBody?> | ||
) { | ||
Log.i(TAG, "shilin>>resp from AF:"+ response.body()?.string()) | ||
//System.out.println(">>>>>>>>>>>>"+response.body()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@FormUrlEncoded | ||
@POST("consumption-reporting/{aspId}") | ||
fun postConsumptionReporting(@Path("aspId") aspId: String?, @Field("data") data: String?): Call<ResponseBody>? | ||
//fun postConsumptionReporting(@Path("aspId") aspId: String?, @Field("data") data: String ): Call<String>? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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? |
…ngConfiguration.samplePercentage from UInt to Float
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 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 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:
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...) |
2. refine compare between Float 3. refine log
…ssion with Metric 2. Consumption reporting support multi Clients
@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. |
2. print real report data
|
||
fun startConsumptionReportTimer(clientId: Int) { | ||
val timer = Timer() | ||
clientsSessionData[clientId]?.serviceAccessInformationRequestTimer = timer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you overwrite the serviceAccessInformationRequestTimer? This timer is used to re-request the Service Access Information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a typo and it should be consumptionReportingTimer, thanks for finding the bug. Corrected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will address the remaining issues in the changes I am currently making. As this PR is already merged please do not add any more changes at this point as they will be lost (see also my comment at the bottom)
timer.schedule( | ||
object : TimerTask() { | ||
override fun run() { | ||
clientsSessionData[clientId]?.isConsumptionReportByTimer = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consumption reports shall be send periodically:
Reports are sent at periodic intervals determined by the reporting interval attribute of the consumption reporting configuration specified in Table 4.2.3-2.
With this logic it seems like the next report is send only if a message from the MediaStreamHandler was dispatched and not at the predefined interval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that my understanding is not quite the same as yours, and I only found:
- In 4.7.2.3: The Media Session Handler shall periodically check for updated Service Access Information
- In 4.7.4: Upon determining the need to report ongoing 5GMS consumption at periodic intervals determined by the clientConsumptionReportingConfiguration.reportingInterval property.
As I understood, the report is send only if a message from the MediaStreamHandler was dispatched, and the conditions 4.7.4 occur including check the timer at the predefined interval. Am I misunderstood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consumption reports are sent as defined by reportingInterval
. With each report the duration
property is increased, see also here: 5G-MAG/rt-5gms-media-stream-handler#52
return | ||
} | ||
|
||
val provisisioningSessionId: String = "2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this value hardcoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't found the defintion of provisisioningSessionId in TS26.512, I didn't know how to generate it, so I hardcoded first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the parameter is called provisioningSessionId
, you have typos throughout your code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks
@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. |
return true; | ||
} | ||
|
||
// if the generated random number is of a lower value than the samplePercentage value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be re-evaluated for each consumption report while it should only be re-evaluated for each new streaming session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand, it's purpose is to control the percentage of report(to reduce network footprint?)
I think may the description of this part of the spec is very unclear.@rjb1000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, @shilinding. When less than 100% of 5GMS Clients send consumption reports, the load on the 5GMS AF is reduced.
TS 26.512 clause 11.2.3.1 defines ServiceAccessInformaion.clientConsumptionReportingConfiguration.samplePercentage as:
The percentage of media streaming sessions that shall send consumption reports, expressed as a floating point value between 0.0 and 100.0.
When we discussed how to implement this aspect of metrics reporting, we realised that there is an implementation choice about exactly when to evaluate the random number generator that drive this behaviour:
- Generate the random number once for each streaming session. Reporting is then either enabled or disabled for that streaming session.
- Generate the random number each time you are thinking about sending a report.
This is left ambiguous in TS 26.512, and I think that's fine because it is a case where we can leave it to implementation choice and get a similar server load result across an entire population of 5GMS Clients.
For metrics reporting, we chose option 1. I think @dsilhavy's comment is suggesting that we choose option 1 for consumption reporting as well for consistency.
For option 1, there remains an open question in my mind about the implementation. If the random number determines that reporting is disabled for a session, does the 5GMS Client still need to collect samples, just in case the reporting configuration changes? Should our 5GMS Client implementation re-evaluate the random number if the client configuration changes? And, if it then needs to start reporting, does it need to report historic data? (Maybe that applies more to metrics reports than consumption reports, though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably related to the issue created here: #32
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. |
Get consumption report from Media Stream Handler and post to AF via M5.
Resolves #3.