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

MythMediaMonitor: Replace UDisks with UDisks2 #606

Merged
merged 2 commits into from
Jul 25, 2022

Conversation

rcrdnalor
Copy link
Contributor

UDisks is long time deprecated and not available in modern Linux
distributions anymore.
It was replaced by UDisks2 which is now provided by the storaged.org
project and linked from freedesktop.org.
See https://udisks.freedesktop.org/ and
http://storaged.org/doc/udisks2-api/latest/ for details.

When monitoring is enabled in the frontend:
Setup -> General -> Monitor Devices
a 'MythMediaDevice' is added for every removeable device.

Refs #605

Thank you for contributing to MythTV!

Checklist

UDisks is long time deprecated and not available in modern Linux
distributions anymore.
It was replaced by UDisks2 which is now provided by the storaged.org
project and linked from freedesktop.org.
See https://udisks.freedesktop.org/ and
http://storaged.org/doc/udisks2-api/latest/ for details.

When monitoring is enabled in the frontend:
Setup -> General ->  Monitor Devices
a 'MythMediaDevice' is added for every removeable device.

Refs MythTV#605
caused by my previous commit to mediamonitor-unix.

Configuration of clang-tidy and clazy have been taken from the
corresponding '.dot' files with the same name.

Do not forget to enable media monitoring in
mythfrontend: Setup --> General --> Media Monitor
when using this feature.

Refs MythTV#605
@rcrdnalor
Copy link
Contributor Author

@linuxdude42, @paul-h

Could you please review this PR?
When I became a MythTV developer, I promised to write pull requests
for every non-trivial change not related to python.

For testing, you need to enable media monitoring in
mythfrontend: Setup --> General --> Media Monitor
and set frontend logging to 'media' with loglevel 'info' or 'debug'.
It detects now removable devices like DVDs and USB-Sticks which are
visible under 'Optical Disks'.

Not sure, why the two checks for macos 10.15 have been canceled.

Thanks,
Roland

@linuxdude42
Copy link
Contributor

Works great. Recognizes CDs, DVDs, and BDs. When I inserted the CD it started MythMusic for me. I was manually able to start playing the DVD and the BD. It would be nice if the code could be enhanced to ask which disc to play if more than one is inserted, but that's probably a different project.

I tested a couple of USB sticks. I see them in the debug output and can eject them. I don't know what MythTV is looking for on the USB stick, so I haven't tested to see if it can play from USB. Do you know if its looking for a partition that mounts as an ISO, or a partition containing a DVD structure, or a partition containing ISO files or MKV files? I will test more later.

@rcrdnalor
Copy link
Contributor Author

David,
thank you for looking into this topic.
Without any mythtv plugins installed (like mythmusic), I can insert, play and eject a DVD recognized by UDisks2.
If I insert an USB stick or an USB disk drive the log (media, debug) shows a lot of checks.
I think, the class MythMediaDevice is responsible to take over any of the registered actions.
That's why I asked paul-h to join the party. He is the only active developer I know how
could resolve this puzzle, but others are also invited to chime in this topic:
The class 'MythMediaDevice' only knows about 'MythCDROM' and 'MythHDD'.
What to do if the inserted media contains pictures or music instead of video?

IMHO, this is issue is able to trigger -again- some actions in respect to media handling.
But further actions should be done outside this topic.

Thanks again,
Roland

P.S.: We could add check items about 'clazy' and 'clang-tidy' to the 'Checklist' provided by the pull-request template.
I've seen some contributors others like me struggling with these checks. These checks should be optional, but it is
good to know if they passed.

@garybuhrmaster
Copy link
Contributor

P.S.: We could add check items about 'clazy' and 'clang-tidy' to the 'Checklist' provided by the pull-request template. I've seen some contributors others like me struggling with these checks. These checks should be optional, but it is good to know if they passed.

Probably would be best if the github actions on a PR ran a clazy and clang-tidy step to let contributors get feedback as part of the PR checks. I have not looked at what it requires to run either (so do not know how easy creating the action would be), but let me know if the project is interested in having me look at such in my free time (when I find free time).

@linuxdude42
Copy link
Contributor

Interesting. Before when I inserted the USB stick, I got this message:

DetectMediaType No files with extensions found in '/run/media/david/pnyusb'

After copying a video onto the USB stick and reinserting it I saw this messages:

DetectMediaType MEDIATYPE_MVIDEO|MEDIATYPE_MGALLERY (mkv)

@linuxdude42
Copy link
Contributor

Probably would be best if the github actions on a PR ran a clazy and clang-tidy step to let contributors get feedback as part of the PR checks. I have not looked at what it requires to run either (so do not know how easy creating the action would be)

You have to 1) compile mythtv from scratch using clang and some extra flags, 2) generate a compile_commands.json file, and then you can 3) run the tidy or clazy program. I suppose the builders are already doing step one each time since the builds are done in a fresh VM, so you would just need to add steps 2 and 3. Step 2 is already in the makefile as 'make compdb'. I'm not sure if the git builders include Ubuntu the clang-tidy or clazy packages or if they work properly. (I've only ever run tidy/clazy on Fedora, and I've been having to build my own clazy on Fedora as the packaged version always seems to require older clang libs.)

@rcrdnalor
Copy link
Contributor Author

Yes, DetectMediaType is part of the class MythMediaDevice, which should be revised or checked for acting properly.

@rcrdnalor
Copy link
Contributor Author

rcrdnalor commented Jul 22, 2022

Probably would be best if the github actions on a PR ran a clazy and clang-tidy step to let contributors get feedback as part of the PR checks. I have not looked at what it requires to run either (so do not know how easy creating the action would be)

Please don't be to harsh. I meant to allow users to propose changes, even they are are not fully compliant to the rules which mythtv require.

Roland

@garybuhrmaster
Copy link
Contributor

Probably would be best if the github actions on a PR ran a clazy and clang-tidy step to let contributors get feedback as part of the PR checks. I have not looked at what it requires to run either (so do not know how easy creating the action would be)

You have to 1) compile mythtv from scratch using clang and some extra flags, 2) generate a compile_commands.json file, and then you can 3) run the tidy or clazy program. I suppose the builders are already doing step one each time since the builds are done in a fresh VM, so you would just need to add steps 2 and 3. Step 2 is already in the makefile as 'make compdb'. I'm not sure if the git builders include Ubuntu the clang-tidy or clazy packages or if they work properly. (I've only ever run tidy/clazy on Fedora, and I've been having to build my own clazy on Fedora as the packaged version always seems to require older clang libs.)

Running Fedora (latest or rawhide) in git actions is typically not so much of a problem for these types of checks (you just run one of the official Fedora docker containers on the Ubuntu host(s), and I do that for other actions on other projects as part of their build testing). And if the action needs to build the latest tidy/lazy, that would be possible too (some of my actions have to build numerous perl libraries just to test that a trivial program can run its tests; somewhat ugly, but it works). I would even think that the right solution would be to checkout the ansible repo in the action, and build the build environment to do the builds (rather than hard coding the dependencies into the actions, meaning there is one source of "truth" (ansible)). Anyway, I can add the idea to my list of things I am not sure when I will have time to work on (I believe that list may still be countable infinite).

@paul-h
Copy link
Contributor

paul-h commented Jul 24, 2022

The media monitor has not worked properly for so long it's hard to remember how it used to work. I did briefly try the patch and like David it recognised a CD and MythMusic started playing OK albeit without the metadata :(

The only other plugins that should handle media monitor events will be the gallery and video plugins that are now part of MythTV base so not really plugins any more. The way it works is when new media is detected that contains files the media monitor looks at the files present and passes the event on to the plugin that has registered interest in the file extensions present. So if there are music files present then MythMusic will start and add them to a temporary playlist. I guess video files will start the Video plugin and image files will start the gallery. Don't recall how media with mixed files are handled. The other type of media are CD and DVD discs which I believe have different handlers but work in a similar way being passed to the plugin that has registered interest in those types of media.

I would think USB discs are handled the same as a removable HDD?

I can vaguely remember ending up disabling the media monitor because the family had the annoying habit of leaving a CD or DVD in the drive that caused the relevant media handler to kick in whenever the frontend was restarted but that is another story.

The patch looks good to me and can only be an improvement on the old defunct UDisks support we had.

@rcrdnalor rcrdnalor merged commit 55425a9 into MythTV:master Jul 25, 2022
@rcrdnalor
Copy link
Contributor Author

PR merged. I assumed an approval by the reviewers.

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.

4 participants