[Bug 768110] new plugin: ahssrc (Android hardware sensor source)

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768110

--- Comment #16 from Martin Kelly <martin at surround.io> ---
(In reply to Olivier Crête from comment #15)
> Review of attachment 331379 [details] [review]:
> 
> ::: sys/androidmedia/gstahssrc.c
> @@ +613,3 @@
> +  /* If the clock is NULL, the pipeline is not yet set to PLAYING. */
> +  if (pipeline_clock == NULL)
> +    goto done;
> 
> You should also provide the system clock as a backup by implementing the 
> (see how rtpjitterbuffer does it). If you have ahssrc ! appsink, there will
> be no element to provide one.

Could you explain this part more? I see what rtpjitterbuffer is doing but I
don't fully understand why. In my current test app, I'm using a bunch of ahssrc
elements going into a funnel going into an appsink. I'm seeing
GST_ELEMENT_CLOCK return a valid clock that looks to be monotonic with some
base.

In addition, if my element provides a clock, why not choose a monotonic one
rather than the system clock? It seems like that would be more reliable, but
I'm probably missing something here, as I don't fully understand how all the
clocks work in gstreamer.

> @@ +629,3 @@
> +  gst_object_ref (pipeline_clock);
> +  buffer_time = gst_clock_get_time (pipeline_clock);
> +  gst_object_unref (pipeline_clock);
> 
> No need to ref & unref the clock, you're still holding the element's object
> lock.

Agreed; I'll fix that.

> 
> @@ +636,3 @@
> +  else
> +    buffer_duration = GST_CLOCK_TIME_NONE;
> +  self->previous_time = buffer_time;
> 
> I would just skip the duration entirely and always set it to NONE. Those
> events don't really have a duration.

Agreed; I'll fix that.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.