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

Fixing camera handling (libgphoto2) via a background thread #5212

Closed
wants to merge 2 commits into from
Closed

Fixing camera handling (libgphoto2) via a background thread #5212

wants to merge 2 commits into from

Conversation

jenshannoschwalm
Copy link
Collaborator

There are some issues around (#2142 #4917 #4253) related to libgphoto2 camera handling.

What is basically wrong or not working in a good way?

  1. scanning for cameras was done in two ways. a) as a background job once at startup and b)
    via g_idle_add later on, thus possibly blocking the gui thread.

  2. detection of devices to be removed from the current list and the gui import interface only
    tested for the model, not for the port. This left unavailable cameras in the list (easily when using
    two sd cards).

  3. updating cameras in gui was not protected from camera updating

How have these problems been (hopefully) fixed?

  1. Updating the camera list is done in a background thread (bt).

  2. Every 500ms the bt checks for the connected cameras, if any changes are found it
    sends a signal for the gui interface to be updated.

  3. Updating the camera gui interface is protected via the dt_camctl_t mutex.

  4. The rescan button is not necessary any longer so it's gone.

I tested this with cards and cameras i have myself. This all works fine.

Unfortunately i have no camera with tethering so i could not test that case at all.

Please review and test!

Likely
fixes #2142
fixes #4917
fixes #4253

There are some issues around (#2142 #4917 #4253) related to libgphoto2 camera handling.

What is basically wrong or not working in a good way?

1. scanning for cameras was done in two ways. a) as a background job once at startup and b)
 via g_idle_add later on, thus possibly blocking the gui thread.

2. detection of devices to be removed from the current list and the gui import interface *only*
tested for the model, *not* for the port. This left unavailable cameras in the list (easily when using
two sd cards).

3. updating cameras in gui was not protected from camera updating

How have these problems been (hopefully) fixed?

1. Updating the camera list is done in a background thread (bt).

2. Every 500ms the bt checks for the connected cameras, if any changes are found it
sends a signal for the gui interface to be updated.

3. Updating the camera gui interface is protected via the dt_camctl_t mutex.

4. The rescan button is not necessary any longer so it's gone.

I tested this with cards and cameras i have myself. This all works fine.

Unfortunately i have no camera with tethering so i could *not* test that case at all.

Please review and test!

Likely
fixes #2142
fixes #4917
fixes #4253
If there is no gui we have no darktable.camctl so we can't set the stopper flag.
@johnny-bit
Copy link
Member

Wow!

I have a question though - is it possible to have fix for #4380 in it too? I browsed entangle code for that and they detect blocked camera and try to eject it (essentially giving controll back to gphoto for further detection) upon user confirmation.

@jenshannoschwalm
Copy link
Collaborator Author

I guess so. Although I don't understand the benefit of that and I don't know whether all other gphoto based applications behave well.

This stuff has been somewhat complicated to understand and I would rather have it tested well as it is atm especially with tethering used.

Reports about that highly appreciated...

@johnny-bit
Copy link
Member

I have camera capable of tethering :)

So I tested it and Good&bad news:

if darktable is on and I connect camera - darktable takes it before it gets mounted by Gnome, so that's a plus 👍

Importing photos work 👍

In tethering view I can enable preview, but neither "take photo" button nor normally pressing shutter :/ and it seems like gphoto2 god bonkers in tethering because camera tries to save image that wasn't taken ;( 👎

On current master tethering works normally though.
I unfortunately don't have much time today to test WHY tethered shooting doesn't work ;(

@jenshannoschwalm
Copy link
Collaborator Author

While using -d camctl, is the backthtread still running or is it blocked by the tethering camera?

@johnny-bit
Copy link
Member

Huh, I tried testing tethering just now and it worked, both via button and via shutter 0.o

what was different this time, was that camera was grabbed first by gnome, so I had to unmount from gnome and then darktable autodetected it and was fun.

capture.log

with

54,756575 [camera_control] gphoto2 error: Canon EOS Capture nie powiodło się: może brak ostrości?
54,757059 [camera_control] capture job failed to capture image: Nieokreślony błąd

I just had cap on lens trying to take a shoot ;)

@jenshannoschwalm
Copy link
Collaborator Author

For me your capture.log looks fully correct.

So what's the problem if dt gets the camera before gnome?

  1. Does is handle gnome's request in a wrong way? Again what's the log showing ...
  2. Anything else?

@TurboGit TurboGit added this to the 3.4 milestone May 28, 2020
@TurboGit TurboGit self-requested a review May 28, 2020 09:40
@TurboGit TurboGit added bugfix pull request fixing a bug difficulty: average some changes across different parts of the code base feature: redesign current features to rewrite priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: OS support making darktable work on particular operating systems scope: software support wiring external libs and software: LittleCMS, colord, G'MIC, enfuse/enblend, etc. labels May 28, 2020
@TurboGit
Copy link
Member

Nice to see fixes on this area. I'll check at some point as I can do tethering on my side.

@johnny-bit
Copy link
Member

After discussion with @elstoc I couldn't wait despite workload at work and wanted to see if the issue is repeatable and all that.

I need to hand it to @jenshannoschwalm - this PR is NOT at the fault here regarding tethered shooting, it's actually full on gphoto2 problem.
gphoto/libgphoto2#472
gphoto/gphoto2#82
gphoto/libgphoto2#30
gphoto/gphoto2#121

I tried to replicate the issue to no avail, logs clean like the one I posted. I even managed to get darktable to grab camera before gnome and still - everything worked perfectly!

I found that there's a problem with image thumbnails generation, however that's not the fault of this PR either. I filled separate issue: #5219

@ghost
Copy link

ghost commented May 29, 2020

This is likely to interact (improve :-) ) #5161 too even if #5161 turns out to have a different root cause.

TurboGit pushed a commit that referenced this pull request May 30, 2020
This pr is a small version of #5212 intended for 3.2, it does not include the
background thread fully updating the device list.

Done here:
1. Fixes the camera detection. Until now detection of devices to be removed from the current list
and the gui import interface only tested for the model, not for the port.
This left unavailable cameras in the list (easily when using two sd cards)

2. Updating the gui device list is now protected via the camcontrol mutex.

3. Updating the device list so far done by a g_idle defers to a background job so it does
not block the gui any longer.

fixes #2142
fixes #4917
likely fixes #4253 although i could not test with that specific hardware
@jenshannoschwalm
Copy link
Collaborator Author

I will close this for now to keep the pr list small, will come back after 3.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug difficulty: average some changes across different parts of the code base feature: redesign current features to rewrite priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: OS support making darktable work on particular operating systems scope: software support wiring external libs and software: LittleCMS, colord, G'MIC, enfuse/enblend, etc.
Projects
None yet
3 participants