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.

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

Add a 'V' query that will return the maximum seek point in seconds within the current track duration for a 'live' radio stream #986

Merged
merged 6 commits into from Feb 6, 2024

Conversation

expectingtofly
Copy link
Contributor

@expectingtofly expectingtofly commented Jan 30, 2024

As per the discussion here : https://forums.slimdevices.com/forum/developer-forums/developers/1667479-live-streams-tag

This adds a 'V' : Live_Edge query.

It will contain the maximum seek point in seconds of the currently live radio stream that has a defined duration.

This adds the ability for a client to know what is the maximum available downloadable data.
It also provides the information to a controller to display the maximum seek point.

For Example for the following scenario :
A live radio stream could be streaming a programme that is 2 hours long.
The programme is currently being broadcast live and is 1 hour into the broadcast. The Listener is currently 50 minutes into listening to the programme as they paused for 10 minutes.

The query that contained the tags:dV (duration and live_meta) would return:
result->time : 3000
result->remoteMeta->duration : 7200
result->remoteMeta->live_edge : 3600

…thin the current track duration for a 'live' radio stream.
@expectingtofly expectingtofly changed the title Add a 'V' query that will return the maximum seek point in seconds within the current track duration for a 'live radio stream Add a 'V' query that will return the maximum seek point in seconds within the current track duration for a 'live' radio stream Jan 30, 2024
@mherger
Copy link
Contributor

mherger commented Jan 31, 2024

Just for my understanding: Is live_edge a technical term? Where is it coming from?

And the value would need to be exposed by the protocol handler - which currently would be some select 3rd party plugins only? It might be worth adding a note so this doesn't get removed because no code providing it can be found. The risk is small, but it helps understanding how this works.

@expectingtofly
Copy link
Contributor Author

Just for my understanding: Is live_edge a technical term? Where is it coming from?

Yes, it is used in the hls and mpeg-dash specifications to describe the most recently available audio chunk/segment available in a dynamic live audio stream.
Googling 'mpeg-dash or hls live edge' returns lots of hits. So AFAIK it's widely used and neatly describes the maximum seek point in a dynamic live stream.

And the value would need to be exposed by the protocol handler - which currently would be some select 3rd party plugins only? It might be worth adding a note so this doesn't get removed because no code providing it can be found. The risk is small, but it helps understanding how this works.

Yes, that occurred to me, where do you want the note, I have added the item to the cli_api documentation.
Shall I put something in there, or do you want it next to the code?
I guess it wouldn't be used within LMS until/if we implement hls or mpeg-dash in LMS.

@mherger
Copy link
Contributor

mherger commented Jan 31, 2024

Please add the note next to the code. Thanks!

@expectingtofly
Copy link
Contributor Author

Please add the note next to the code. Thanks!

Sure, no problem, branch updated.

@philippe44
Copy link
Contributor

philippe44 commented Jan 31, 2024

I was also initially thinking about that as a flag for webradio to really indicate that this is a really live stream. So it would not be just be initialized by plugins, but LMS would set it to 0 when it's a a true live webradio/content (RP interactive for that matter would not be). Plugins could overwrite it of course. If it's not live, it could be either missing or -1. When it's a live_edge as you describe it, it then as a value > 0

@expectingtofly
Copy link
Contributor Author

I was also initially thinking about that as a flag for webradio to really indicate that this is a really live stream. So it would not be just be initialized by plugins, but LMS would set it to 0 when it's a a true live webradio/content (RP interactive for that matter would not be). Plugins could overwrite it of course. If it's not live, it could be either missing or -1. When it's a live_edge as you describe it, it then as a value > 0

Ok, I think I follow. So basically you would like an indicator that shows that it is a continuous infinite stream.
Whereas I was thinking it was an indicator for only adaptive streams where it is not obvious that it is continuous/live because it is segmented into durations.

Looking back at the forum discussion, I think your original suggestion better meets your needs. I think it got sidetracked by my desire to expose the live edge through LMS

How about then, I amend the PR so that we are back to remoteMeta->{live}
That will contain 0 or be not there if not live ( not continuous)
It will contain >0 if it is live.
So we will populate it with 1 if the stream is live (duration of 0 for internal, isLive for handler) or populate it with a {live_edge} value if it is returned in the getMetaDataFor of a handler

So a non-zero value means it is live, a value greater than 1 means its a live edge

That way it can just be tested for true if the client is not interested in a live edge.

@philippe44
Copy link
Contributor

philippe44 commented Jan 31, 2024

Yes, although we twist the meaning of the value by saying that 1 is just live and >1 is real edge. We can do that, but it's not my preferred semantic I'd say. I'd rather have : nothing or -1, it's not live, anything else give you the distance from the edge, which is 0 in case of webradios.

@expectingtofly
Copy link
Contributor Author

Yes, although we twist the meaning of the value by saying that 1 is just live and >1 is real edge. We can do that, but it's not my preferred semantic I'd say. I'd rather have : nothing or -1, it's not live, anything else give you the distance from the edge, which is 0 in case of webradios.

Well, I think its my fault I'm conflating the two purposes that is causing the issue. I think we are always going to have an odd semantic.
I think I'll submit two pull requests, I'll amend this one just to be for the live edge with a different query tag. And a submit a different pull request just for the your original needs which was for a live ('v') tag that indicated whether live or not.

@philippe44
Copy link
Contributor

Ok, I really we could have one because the meaning is the same
How far are you from the edge ? On a radio 0? You're at the edge. On a dash/hls? Then give you that. -1 means there is no edge at all

@expectingtofly
Copy link
Contributor Author

Ok, I really we could have one because the meaning is the same How far are you from the edge ? On a radio 0? You're at the edge. On a dash/hls? Then give you that. -1 means there is no edge at all

OK, I see your logic. I'll update the branch.

@michaelherger
Copy link
Member

I need to catch up... is the continued discussion one about how to provide the actual value from within the 3rd party plugin, or is it related to this PR? Should there be code in core LMS validating the values returned? Or at least some documentation about what that value is supposed to be for future reference?

@philippe44
Copy link
Contributor

philippe44 commented Feb 4, 2024

I need to catch up... is the continued discussion one about how to provide the actual value from within the 3rd party plugin,
or is it related to this PR?

A bit of both: deciding what that item must be and how it should be calculated

Should there be code in core LMS validating the values returned?

Not for validation, but for default values (LMS knows when it's a live radio)

Or at least some documentation about what that value is supposed to be for future reference?
Yes

@expectingtofly
Copy link
Contributor Author

I'm going to update this branch as per @philippe44 's description

@expectingtofly
Copy link
Contributor Author

expectingtofly commented Feb 4, 2024

Ok, I've updated the Branch.

The live_edge 'V' tag query now returns:
-1 if it is not a live stream
0 if it is live (at the live edge)
greater than 0 representing the number of seconds from the live edge for a dynamic stream protocol handler

I have a couple of questions.

  1. For internal LMS streams I am using $song->isLive() to determine if it is live. This appears to be a reliable way of identifying if is live. I tried combining that with $song->duration() but that was not reliable. Quite often a live http stream has a fixed duration even though it is live. $song-isLive() on its own was consistently correct. Am I right?
  2. As suggested, I have added support for a ->can('isLive') on the protocol handler so a 3rd party plugin can return whether a stream is live and return the live_edge in the metadata. However, is there any point in that handler->islive() ? Should a 3rd party plugin support populating $song->isLive() anyway, making the extra handler operation pointless?

$remoteMeta->{V} = -1;
}
} else { # only live if the song has isLive set
$remoteMeta->{V} = $song->isLive() ? 0 : -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't you set that as the default value and override it only if the handler has something to say? Feels easier to read to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, fair point.

@philippe44
Copy link
Contributor

philippe44 commented Feb 5, 2024

To your question about use of isLive(), I would agree with you. We should not combine song->isLive() with duration(), because the requestor will have the duration() anyway so isLive() basically tells him either the HTTP response when there is no PH did not give a Content-Length (which is not a perfect indication of live either, as HTTP clearly allows server to no disclose CL) or that the PH clearly said it's a live content, although I may set duration. So that gives 2 independent data point to the requestor to make a decision on how it want to handle that track. Otherwise, it would be masked by the lack of duration()

@@ -5010,6 +5011,22 @@ sub _songData {
$remoteMeta->{T} = $remoteMeta->{samplerate};
$remoteMeta->{I} = $remoteMeta->{samplesize};
$remoteMeta->{W} = $remoteMeta->{releasetype};

$remoteMeta->{V} = -1;
# Distance from the live edge of live remote stream. -1 is not live, 0 is live at the edge, >0 is distance in seconds from the live edge.
Copy link
Member

Choose a reason for hiding this comment

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

Please use tabs instead of space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops...

Comment on lines 5017 to 5018
if ( my $client = $request->client ) {
if (my $song = $client->currentSongForUrl($url)) {
Copy link
Member

Choose a reason for hiding this comment

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

Both $client and $song are used in other places of this sub. Please put the definition of the two at the top and remove this repeated evaluation of the two values from wherever it's done now (within this sub, eg. line 5017).

Copy link
Member

@michaelherger michaelherger left a comment

Choose a reason for hiding this comment

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

Please document the meaning of live_edge values as well as the meaning of isLive(). As this is all outside core LMS we don't have code as reference to replace missing documentation. Thanks!

@expectingtofly
Copy link
Contributor Author

To your question about use of isLive(), I would agree with you. We should not combine song->isLive() with duration(), because the requestor will have the duration() anyway so isLive() basically tells him either the HTTP response when there is no PH did not give a Content-Length (which is not a perfect indication of live either, as HTTP clearly allows server to no disclose CL) or that the PH clearly said it's a live content, although I may set duration. So that gives 2 independent data point to the requestor to make a decision on how it want to handle that track. Otherwise, it would be masked by the lack of duration()

Yes, indeed, although I was in particular referring to the $song->duration, which, although I find a bit confusing is a bit different to the duration that is returned by the d query tag. The reason I raised it was because the only other place that isLive is used internally, it combines it with the $song->duration(), which for the reason you describe, I found to be flawed.
Here is the other location it is used in the streamingcontroller :
https://github.com/Logitech/slimserver/blob/ad205f642864c3e12498579a4633ee376b23e644/Slim/Player/StreamingController.pm#L917

But anyway, I think after you have furthered my understanding I definitely think there is no need for the handler->isLive now, so I will remove that from my branch, and assume that 3rd party handlers will also populate $song->isLive appropriatly.

@philippe44
Copy link
Contributor

I'm not sure I understand your comment about song->isLive overload

@expectingtofly
Copy link
Contributor Author

I'm not sure I understand your comment about song->isLive overload

The original forum discussion suggested the 3rd party handler havein its own isLive. I originally had that in the branch. Now we know we are using the song->isLive on its own, then there is no need for it.

@expectingtofly
Copy link
Contributor Author

I've updated the branch, hopefully that is more straightforward now and meets requirements.

@expectingtofly
Copy link
Contributor Author

Branch updated with @philippe44 's suggestions

@mherger mherger merged commit 25448be into LMS-Community:public/8.4 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants