Add dlna server capabilities [Draft] by Slesa · Pull Request #525 · tramhao/termusic

Slesa · 2025-06-29T17:01:55Z

So far, this adds only a new view with key '4' the same way as library does. The idea is to select mp3 from a media server via DLNA.

My question is if this would be the right way, or should the collection be added to the database? Maybe refresh would be more complicated then?

* 'dlna-server' of github.com:Slesa/termusic: DLNA server does not display but runs

tramhao · 2025-06-30T08:01:47Z

As the sources are quite different in 1/2/3, I think your idea is the right way to go. Thanks!

hasezoey

Is this PR still a Draft/Work in Progress, or is it really only meant to add the framework for the dlna services? I personally would prefer if either: dont enable and show anything DLNA yet if it is just bare-bones, or add full functionality before merging.
(if it is still a draft/wip, please mark it as such to avoid accidental merging)

Also am i correctly understanding that this PR is aiming to be a DLNA Client/Player?

If my understanding is correct, then i have the following request:
Change the terminology from dlan*server* to dlna*client* or dlna*player* or something along those lines.

Additionally, not really having a specific place in the code:

run clippy and resolve all the warnings (i would recommend to rebase onto master to resolve the rust 1.88 warnings)
change result / error types to use either anyhow or a custom thiserror
some more documentation

PS: i tried to run termusiclib::dlna::discovery::discover_devices() to see if it works, but it does not return any devices by default. This is likely due to using schema- instead of schemas-.
Note that i have a DLNA server running via Jellyfin (plugin) and kodi and gssdp-device-sniffer can find it just fine.

My question is if this would be the right way, or should the collection be added to the database? Maybe refresh would be more complicated then?

Due to the nature of DLNA, i would not recommend putting it into the sqlite database.

hasezoey · 2025-06-30T08:43:38Z

lib/Cargo.toml

 viuer.workspace = true #   = "0.6"
 walkdir.workspace = true #   = "2"
 wildmatch.workspace = true #   = "2"
+xmltree= ">=0.11"


Would quick-xml(which we already use) be too much overhead?

Probably not. I'll have to figure out the differences...

If a bit honesty is allowed: reading XML with a loop instead of parsing the known paths is horrible

hasezoey · 2025-06-30T08:46:31Z

lib/src/config/v2/tui/keys/mod.rs

+    //#[serde(rename = "dlnaserver")]
+    //pub dlna_server_keys: KeysDlnaServer,


Entry for the future?

Not sure which keys will be necessary... server refresh?

Not sure which keys will be necessary... server refresh?

You will likely want at least the following keys:

load / load_all, similar to the ones already in library (maybe we should also just re-use those keys?)

search, similar to the ones in library (maybe we should also just reuse that?)

some key to open a popup or something to change the dlna server selection? (or will this be part of the tree?)

And yes refresh would also be great. We also need that in the music library, where it currently does not exist (maybe reuse that key then?).

TL;DR: i just asked because it is commented-out code, if it is there for the future when there are keys to be added.

hasezoey · 2025-06-30T08:47:32Z

lib/src/config/v1/key.rs

    pub global_config_open: BindingForEvent,
    pub global_save_playlist: BindingForEvent,
    pub global_layout_podcast: BindingForEvent,
+    pub global_layout_dlnaserver: BindingForEvent,


FYI, the keys dont need to be implemented for v1 config, as that only exists to migrate to v2 if such a config is still found.

I simply tried to be as near to all other modules as I can, so it can surely be removed.

tui/src/ui/components/footer.rs

hasezoey · 2025-06-30T08:55:04Z

tui/src/ui/model/mod.rs


    pub layout: TermusicLayout,
    pub library: MusicLibraryData,
+    pub media_server: MediaServerData,


I would recommend to include dlna in the name. This might not be a problem now, but if in the future things like jellyfin are added, it might get confusing.

I would also recommend to include 'music' in the library field. Names - so hard to find... dlna_media_server then? Not sure if that fits at all. And if it is necessary when writing to the db?

I would also recommend to include 'music' in the library field.

I totally agree there.

Names - so hard to find... dlna_media_server then? Not sure if that fits at all.

Sure. Worst case, we will just change it later.

And if it is necessary when writing to the db?

What exactly do you mean here?

lib/src/dlna/media_server.rs

tui/src/ui/components/footer.rs

lib/src/lib.rs

hasezoey · 2025-06-30T09:53:11Z

tui/src/ui/model/update.rs

+            GSMsg::PopupShowDlnaServer => {
+                self.mount_search_database();
+                self.database_update_search("*");
+            }


This seems to not be trigger-able currently, also i think searching the database in this case would not be correct and would need a custom search implementation.

hasezoey · 2025-06-30T10:46:03Z

lib/src/dlna/discovery.rs

+pub  async fn discover_devices() -> Result<Vec<DlnaDevice>, Box<dyn Error>> {
+    let mut dlna_devices = Vec::new();
+
+    let search_target = URN::device("schema-upnp-org", "MediaServer", 1).into();


Suggested change

let search_target = URN::device("schema-upnp-org", "MediaServer", 1).into();

let search_target = URN::device("schemas-upnp-org", "MediaServer", 1).into();

From my basic knowledge of DLNA, this should be schemas instead of schema.
This way it actually works and finds my jellyfin DLNA server.

I did not find the typo, so I copied the already running line from my PoC. But thanks.

hasezoey · 2025-06-30T11:11:59Z

tui/src/ui/components/dlna_server.rs

+        } else {
+            (
+                CmdResult::None,
+                Some(Msg::Playlist(PLMsg::Add(path.to_path_buf()))),


I think i forgot to mention this in my review, but this message type should likely be refactored to allow other things than a Path, as to my understanding DLNA playback is done via http streams. (also we should add a way to add radios, which i dont think exists yet in the TUI)
As for this PR, it should likely either do that refactor, or at least note a TODO about this.

As a workaround, I thought about a temporary download of the file from the server.

…w and unsorted.

Slesa · 2025-06-30T22:41:58Z

Is this PR still a Draft/Work in Progress, or is it really only meant to add the framework for the dlna services? I personally would prefer if either: dont enable and show anything DLNA yet if it is just bare-bones, or add full functionality before merging. (if it is still a draft/wip, please mark it as such to avoid accidental merging)

If I would have been sure that this is the right way how to implement it, I would have done it on my own, nothing but me and my laptop until its done. But as long as I have so many questions, I thought it would be better to show it to those who have already implemented so much and ask them if it is the right way.

Thank you for all your code review work, I appreciate that. But maybe it was ~12h too early ;-) at least for the discover stuff.

Also am i correctly understanding that this PR is aiming to be a DLNA Client/Player?

As a first step, I wanted to collect the mp3 collection from the MediaServer and try to play the songs locally. In a second step, I would like to send them to a MediaRenderer, too.

If my understanding is correct, then i have the following request: Change the terminology from dlan*server* to dlna*client* or dlna*player* or something along those lines.

Probably the dlna_player will be the renderer, won't it? So dlna_client would be best? Or keep the terms of upnp with dlna_media_server and dlna_media_renderer?

run clippy and resolve all the warnings (i would recommend to rebase onto master to resolve the rust 1.88 warnings)

change result / error types to use either anyhow or a custom thiserror

some more documentation

Yes, this can all be done. But please let me fix all the problems first.

My question is if this would be the right way, or should the collection be added to the database? Maybe refresh would be more complicated then?

Due to the nature of DLNA, i would not recommend putting it into the sqlite database.

And then show everything in the music library tab with all other tracks? Is it possible to play the tracks directly with the http link? Maybe the covers should be supported, too? But a refresh is probably slow then, as every entry has to be checked if it has changed, has been removed or is new.

Right now, the reading of the complete collection is really fast, as I simply read the All Music container. But I have problems to split it into the tree folder. Would it be better to allow
All Music (with sort)
Albums
Artists
and read the children when opening the tree as all others to?

hasezoey · 2025-07-01T08:52:58Z

But as long as I have so many questions, I thought it would be better to show it to those who have already implemented so much and ask them if it is the right way.

This is good, but without marking it draft it could be accidentally merged, so i wanted to be sure what approach this PR takes. It also sets the expectations of this PR in terms of its finished state.

In a second step, I would like to send them to a MediaRenderer, too.

I dont know much about DLNA, wouldnt MediaRenderer be similar to MPRIS, as in, be a playback controller instead of doing the playback (decode and output)?

Probably the dlna_player will be the renderer, won't it? So dlna_client would be best? Or keep the terms of upnp with dlna_media_server and dlna_media_renderer?

I am still not familiar with how DLNA / UPNP works and its terminology, but to my knowledge the MediaServer is the thing that advertises and provides access to the files (ie jellyfin in my case), whereas this PR seems to be the consumer, ie the client connecting to the MediaServer.

If my quick search is correct, the following terminology applies to DLNA:

MediaServer: the server that provides access to the files
MediaRenderer: the controller for playback of a client
(unclear; the client): the client that connects and interacts with the MediaServer

Due to the nature of DLNA, i would not recommend putting it into the sqlite database.
And then show everything in the music library tab with all other tracks?

I dont think this would be a good idea to just put it there "as is", the new tab is fine, but maybe in the future, if wanted, we could do something similar to what kodi does: provide "sources" in the tree, like "files", "dlna", etc.

Is it possible to play the tracks directly with the http link?

Yes that is currently supported, though it is tailored to radio playback currently, so it might need a bit of extra work to get metadata, length, etc; but playback should be fine.

But a refresh is probably slow then, as every entry has to be checked if it has changed, has been removed or is new.

Do you mean changed / refresh the list the dlna server provides? Wouldnt we need to do this anyway?

Right now, the reading of the complete collection is really fast, as I simply read the All Music container. But I have problems to split it into the tree folder. Would it be better to allow
All Music (with sort)
Albums
Artists
and read the children when opening the tree as all others to?

I would take it as other players do and provide those options and load-on-demand, like we do with the music library (files) currently. (maybe cache the responses somehow though)

@Slesa Slesa changed the title Add dlna server capabilities Add dlna server capabilities [Draft] Jul 1, 2025

I meant to convert the PR status to draft:

This would provide:

lib/src/dlna/models.rs

hasezoey · 2025-07-01T09:12:46Z

tui/src/ui/components/dlna_server.rs

+            let album = item.album.unwrap_or("Unknown album".to_string());
+            let artist = item.artist.unwrap_or("Unknown artist".to_string());


Consider using the constants from lib::library_db::track_db (i know they are in the library currently, but with #511 they would be moved, and they are also currently used elsewhere).

hasezoey · 2025-07-01T09:13:30Z

tui/src/ui/components/dlna_server.rs

+            if songs > 1000 {
+                break;
+            }


Is this limiting just for quick testing?

Yes, as long a I am not sure how to deal with albums/artists

hasezoey · 2025-07-01T09:14:47Z

tui/src/ui/model/mod.rs

        );

        let media_root = Self::loading_tree();
+        Self::discover_servers(tx_to_main.clone()).await;


I would recommend to put this as a tokio task instead of forcing to wait for DLNA before showing the TUI.

I tried so, but it always panics. Do you may have an explanation why? I used the runtime with block_on, and as soon as an await was in the call it crashed.

I used the runtime with block_on, and as soon as an await was in the call it crashed.

I dont quite understand what you are trying. It should be as easy as:

tokio::spawn(Self::discover_servers(tx_to_main.clone()));

Or with error logging or other processing:

tokio::spawn(async move { // the clone may need to happen outside of this block though if let Err(err) = Self::discover_servers(tx_to_main.clone()).await { error!("DLNA discover error: {err:?#}"); } });

See for example how songtag download works.

This is strange... when I use tokio::spawn, I get "future cannot be sent between threads". When I use it exactly as in the songtext with tokio::task::block_in_place, then it does exactly what it says: it blocks., so the starting is delayed, the ui is shown later.

I get "future cannot be sent between threads".

Assuming you have not changed it already and even without seeing the full error, i can image this is because of your Error type. Try changing your Result<_, Box<dyn Error>> to anyhow::Result<_> and try tokio::spawn again.
If this does not work, then there is also the option of tokio::task::spawn_local.
...Or try Box<dyn Error + Send>, but i still recommend to use the standard error type we use across all the crates (anyhow::Result) or a totally custom one via thiserror.

When I use it exactly as in the songtext with tokio::task::block_in_place, then it does exactly what it says: it blocks., so the starting is delayed, the ui is shown later.

Also practically doing the same as just doing a plain .await as currently, but in more steps.

Slesa added 6 commits June 24, 2025 23:37

Add dlna server window

d3223a8

DLNA server does not display but runs

44a9805

Add additional view for DLNA server selection

4f6ebb5

Merge branch 'dlna-server' of github.com:Slesa/termusic into dlna-server

87be43c

* 'dlna-server' of github.com:Slesa/termusic: DLNA server does not display but runs

Introduce DLNA access to media servers

526d83c

Try adding a model for media server

266731f

hasezoey requested changes Jun 30, 2025

View reviewed changes

hasezoey reviewed Jun 30, 2025

View reviewed changes

Tracks are collected from MediaServer, but as plain list w/o tree vie…

808f87c

…w and unsorted.

Slesa changed the title ~~Add dlna server capabilities~~ Add dlna server capabilities [Draft] Jun 30, 2025

hasezoey reviewed Jul 1, 2025

View reviewed changes

Slesa marked this pull request as draft July 1, 2025 09:41

Read in remote directories from DLNA server

6d1c058

		//#[serde(rename = "dlnaserver")]
		//pub dlna_server_keys: KeysDlnaServer,

	let search_target = URN::device("schema-upnp-org", "MediaServer", 1).into();
	let search_target = URN::device("schemas-upnp-org", "MediaServer", 1).into();

		let album = item.album.unwrap_or("Unknown album".to_string());
		let artist = item.artist.unwrap_or("Unknown artist".to_string());

Comments

Conversation

Slesa commented Jun 29, 2025

Uh oh!

tramhao commented Jun 30, 2025

Uh oh!

hasezoey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasezoey Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Slesa commented Jun 30, 2025

Uh oh!

hasezoey commented Jul 1, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasezoey Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasezoey Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hasezoey Jul 1, 2025 •

edited

Loading

hasezoey Jul 1, 2025 •

edited

Loading

hasezoey Jul 4, 2025 •

edited

Loading