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

[upnpcontrol] Support for more audio streams through the HTTP audio s… #15122

Merged
merged 2 commits into from
Jul 2, 2023
Merged

[upnpcontrol] Support for more audio streams through the HTTP audio s… #15122

merged 2 commits into from
Jul 2, 2023

Conversation

lolodomo
Copy link
Contributor

…ervlet

Related to #15113

Signed-off-by: Laurent Garnier [email protected]

@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Jun 19, 2023
@lolodomo lolodomo requested a review from mherwege as a code owner June 19, 2023 19:36
@lolodomo lolodomo mentioned this pull request Jun 19, 2023
Closed
13 tasks
lolodomo
lolodomo commented Jun 19, 2023
View reviewed changes
...pcontrol/src/main/java/org/openhab/binding/upnpcontrol/internal/audiosink/UpnpAudioSink.java Outdated
}
playMedia(url);
try {
audioStream.close();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dalgwen : please confirm that it is ok to close the audio stream at this place (async handling).

Copy link
Contributor

Choose a reason for hiding this comment

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

You can, but I think it's better not to :

  • You can do this, because your AudioSink asks the servlet to make it a multi time stream.
    And thus, all call to the servlet will clone the stream, and the original stream won't be needed. So it's OK if you close it.

  • You don't have to, because the AudioServlet will close it (when removeTimedOutStreams or createClonableInputStream is called). And since the rewrite, removeTimedOutStreams is always called (before, it was not and it was a potential resource leak)

  • Please also note that for Audio Sink that do not require multi time stream, this is a faulty behaviour. A one time stream is directly used by the servlet to give audio data to the requesting device, and thus should not be closed.

So I suggest not closing it, to have a consistent code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fine.

I will keep this only for the case URLAudioStream.

@mherwege
Copy link
Contributor

I did not test this, and I don’t have an answer to your question about the right place to close the stream. Apart from that LGTM. @dalgwen should comment.

@dalgwen
Copy link
Contributor

dalgwen commented Jul 1, 2023 •
edited
Loading

LGTM.
And I tested it and it works.
Please note that the volume restoration is not perfect (the 20 seconds timeout is also a low limit : volume restoration will not occur before this, even if the sound is shorter). May I suggest to lower it ? I think that if the media renderer take longer than 10 seconds (even 5?) to initiate the sound, it is likely to not succeed at all.
The timeout is only for the sound initialisation (first doGet() from the device to the servlet), if the sound is longer, it is not an issue.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 1, 2023 •
edited
Loading

Please note that the volume restoration is not perfect (the 20 seconds timeout is also a low limit : volume restoration will not occur before this, even if the sound is shorter). May I suggest to lower it ? I think that if the media renderer take longer than 10 seconds (even 5?) to initiate the sound, it is likely to not succeed at all.
The timeout is only for the sound initialisation (first doGet() from the device to the servlet), if the sound is longer, it is not an issue.

Ok, this timeout is a low limit for volume restoration . But imagine that if the notification takes 30s for example, the volume restoration will occur after the end of the notification, not after 20s ?
So I understand that we have to reduce this timeout at most of possible. But I know that Sonos for example takes a moment to start playing (not sure how many seconds), maybe we have a risk with 5s. That being said, Sonos binding is handling volume restoration by its own as it is a synchronous handling.
So we set it to 5s everywhere ?

@lolodomo lolodomo force-pushed the upnpcontrol_sink branch from dad0f8f to d3c6380 Compare July 1, 2023 07:58
@dalgwen
Copy link
Contributor

dalgwen commented Jul 1, 2023

But imagine that if the notification takes 30s for example, the volume restoration will occur after the end of the notification, not after 20s ?

Exactly. The audio sound will be available on the HTTP server for at least 20 seconds. And if a doGet() occurs during this 20 seconds, il will stay as long as it is needed to play it entirely, be it 30 seconds or one hour, by trying to guess the duration and making the assumption that the sound starts playing as soon as the doGet() occurs.

So we set it to 5s everywhere ?

It should be binding dependant. I think Squeezebox binding is pretty slow to start for example.
For upnpcontrol, I don't have many experience with it. I just tried with a software kodi and it was very fast. WDYT ?

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 1, 2023

@openhab/add-ons-maintainers : I believe this PR is ready for a merge.

@kaikreuzer kaikreuzer merged commit 676f53b into openhab:main Jul 2, 2023
@kaikreuzer kaikreuzer added this to the 4.0 milestone Jul 2, 2023
@lolodomo lolodomo deleted the upnpcontrol_sink branch July 2, 2023 09:41
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Jul 8, 2023
@lolodomo @markus7017
[upnpcontrol] Support for more audio streams through the HTTP audio s… (
889b5a3 
openhab#15122)

* [upnpcontrol] Support for more audio streams through the HTTP audio servlet

Related to openhab#15113

Signed-off-by: Laurent Garnier <[email protected]>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Aug 9, 2023
@lolodomo @matchews
[upnpcontrol] Support for more audio streams through the HTTP audio s… (
f38dbfc 
openhab#15122)

* [upnpcontrol] Support for more audio streams through the HTTP audio servlet

Related to openhab#15113

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
@lolodomo @austvik
[upnpcontrol] Support for more audio streams through the HTTP audio s… (
37d612c 
openhab#15122)

* [upnpcontrol] Support for more audio streams through the HTTP audio servlet

Related to openhab#15113

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalgwen dalgwen dalgwen left review comments

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@mherwege mherwege Awaiting requested review from mherwege mherwege is a code owner

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lolodomo @mherwege @dalgwen @kaikreuzer