Finalize recording state policy

[matze]

This change fixes and unifies handling of different "is-recording" states. First
of all there are potentially two different states: the priv->is_recording state
from the base class and some device-specific state. Device authors can override
the "is-recording" property to provide device-specific information. This change
sets the global recording state as follows:

1. uca_camera_is_recording returns the value of ::is-recording.
2. priv->is_recording is the default state that is returned for ::is-recording
   and set after successfully calling uca_camera_start_recording and
   uca_camera_stop_recording.
3. uca_camera_start_recording, uca_camera_stop_recording, uca_camera_trigger
   use ::is-recording to determine how to proceed.
4. For performance reasons, uca_camera_grab relies on priv->is_recording.

[matze]

Related to #44.

@Tomago: please run the branch and check if it works. Superficial eye review is not going to make it this time.

[tfarago]

If you tell me where libuca on the camera1 machine is. It is not in /opt

[tfarago]

Never mind, found it.

[tfarago]

$ uca-camera-control -c pco
Segmentation fault

[matze]

On 08/13/14 13:24, Tomas Farago wrote:

If you tell me where libuca on the camera1 machine is. It is not in /opt

It's in /root/libuca.

[matze]

Could you give me a bit more detailed information? For example by giving me the output of

$ gdb uca-camera-control -c pco
> r -c pco
> bt

[tfarago]

There is tens of thousands of lines, but they all repeat this pattern:

#28482 0x00007ffff57de837 in g_object_get () from /usr/lib64/libgobject-2.0.so.0
#28483 0x00007ffff78d7036 in uca_camera_is_recording () from /usr/lib64/libuca.so.2
#28484 0x00007fffeeb0e1cf in uca_pco_camera_get_property () from /usr/lib64/uca/libucapco.so
#28485 0x00007ffff57de3ae in g_object_get_valist () from /usr/lib64/libgobject-2.0.so.

[tfarago]

This would require this change to uca-net (git diff, I think it says it best):

 uca_net_camera_dispose (GObject *object)
 {
-    g_object_unref (UCA_NET_CAMERA_GET_PRIVATE (object)->client);
+    UcaNetCameraPrivate *priv;
+
+    priv = UCA_NET_CAMERA_GET_PRIVATE (object);
+
     G_OBJECT_CLASS (uca_net_camera_parent_class)->dispose (object);
+    g_clear_object (&priv->client);
 }

the change from unref to clear is not so huge, but unless we release the connection after we call parent, the is-recording check will try to use the released connections, jajks. I didn't read the docs too carefully yet, maybe @matze can comment whether this will doom us completely or not. The alternative is to a) release the client in the finalize or b) do not stop recording in parent.

[tfarago]

The segfault came from both btw, the error being freed when not initialized and the connection usage after freeing it, that's fixed now.

[matze]

I didn't read the docs too carefully yet, maybe @matze can comment whether this will doom us completely or not.

You are half-way there. g_clear_object is used to unref an object or no-op if the pointer is NULL, so that is good. But you should definitely swap the order of clearing and calling the parent disposal method.

[tfarago]

yep, but then the recording cannot be stopped in the parent, because it will try to use our freed connection.

[matze]

Then do the obvious thing, reset the state properly when destroying the object so that calling stop will not use any freed connection.

[tfarago]

Sure, when I find 10 minutes between other things. Just FYI: it's not about the stop but about the uca_camera_is_recording, which tries to get the property from ucad.

[tfarago]

Done there.

[tfarago]

4. or performance reasons, uca_camera_grab relies on priv->is_recording.

When I kill a concert session (no stop recording) and start it again, the state is correct but the grab will fail because of this. But sure, we cannot be checking over network all the time if the camera is recording. This should be addressed before we merge.

[tfarago]

Will I go to hell?

[tfarago]

Does it mean you like it or I will? 😄 Anyway, it's not good yet, more on Monday...

[tfarago]

Got rid of this.

[tfarago]

This solves point 4.

[tfarago]

Todo: make sure the property is consistent with the parameter. Right now child's property getter will not update the cached state, whereas call to is_recording will.

[tfarago]

Alright, so now the camera calls plugin getters when the construction is done to make sure that the cached states (priv->is_recording and priv->is_readout) are the ones obtained from actual devices. This way we make sure that when we construct the object we are synced with the actual device and it allows e.g. to use the net camera after client crash without restarting ucad. I am still a total rookie regarding GObject but from what I read constructed() seems like an appropriate place we can do this and the non-construct-properties do not matter since is-readout and is-recording are "computed".

Some remarks:

• After construction, the cached value can go out-of-sync with the getter on special occasion (camera stopped recording automatically because buffer is full), but this must be detectable by the camera and/or its user code and recoverable
• I propose to change 3. and use priv->is_recording also in trigger(), I can imagine that one wants to quickly trigger & grab

[tfarago]

@matze, does your silence mean approval or the opposite? Or you don't like it but don't have a better solution yet? 😄

[tfarago]

By the way, if we merge this it will probably go to the Guiness world records book for the longest open PR 😄

[matze]

@matze, does your silence mean approval or the opposite?

There is only noise coming from me when confused ;-) so take my approval!