GNOME Bugzilla – Bug 763308
ahcsrc: failed to run repeated playing and paused actions
Last modified: 2018-11-03 13:47:30 UTC
When "sync" property is set as "false", the pipeline seems to work well. However, if sync is true on sink element, the pipeline including ahcsrc fails to change state to PLAYING. Reproducible sequence: - Run android camera example apps (#760963) - need to confirm sync property is true - Repeat click paused and play Then - Cannot transit to playing state Tested on Nexus 5x and LG V410 (I am not sure if this error is occurred in other devices)
Created attachment 324406 [details] [review] adjust latency Proposed patch can soothe this problem, but I think it's not a fix for root cause.
Review of attachment 324406 [details] [review]: ::: sys/androidmedia/gstahcsrc.c @@ +2500,3 @@ GST_DEBUG_OBJECT (self, "Reporting latency min: %" GST_TIME_FORMAT, GST_TIME_ARGS (min)); + gst_query_set_latency (query, TRUE, min * 5, min * 5); Where does this 5 come from?
Review of attachment 324406 [details] [review]: ::: sys/androidmedia/gstahcsrc.c @@ +2500,3 @@ GST_DEBUG_OBJECT (self, "Reporting latency min: %" GST_TIME_FORMAT, GST_TIME_ARGS (min)); + gst_query_set_latency (query, TRUE, min * 5, min * 5); I am not sure this is related to the number of callback buffers. But I used the value of it. > 157 #define NUM_CALLBACK_BUFFERS 5 If less than 5, it didn't work well.
What does that NUM_CALLBACK_BUFFERS mean? Is it the number of buffers we allocate, and if all 5 are somehow in use we can't capture anymore? In that case it seems more like something for the maximum latency value. For the minimum we should somehow get the maximum time that is taken from timestamping the frame to having it inside ahcsrc.
Tbh, the purpose of this patch is somewhat to get advise. (In reply to Sebastian Dröge (slomo) from comment #4) > What does that NUM_CALLBACK_BUFFERS mean? Is it the number of buffers we > allocate, and if all 5 are somehow in use we can't capture anymore? It seems to be right. All of allocated buffers are in use even in PAUSED status and some buffers would be discarded by some reasons (not yet analysed). http://developer.android.com/reference/android/hardware/Camera.html#addCallbackBuffer(byte[]) > In that case it seems more like something for the maximum latency value. In my testing on my two devices (Nexus 5x and LG V410), maximum latency value has no effect. > For the minimum we should somehow get the maximum time that is taken from > timestamping the frame to having it inside ahcsrc. I agree and am trying to get proper values, but it's still vague.
(In reply to Justin J. Kim from comment #5) > > In that case it seems more like something for the maximum latency value. > > In my testing on my two devices (Nexus 5x and LG V410), maximum latency > value has no effect. Maximum latency is currently mostly a informational value. Would've been surprising if it had an effect. > > For the minimum we should somehow get the maximum time that is taken from > > timestamping the frame to having it inside ahcsrc. > > I agree and am trying to get proper values, but it's still vague. When are buffers timestamped in the source? Just when they arrive in ahcsrc or by Android before that already?
(In reply to Sebastian Dröge (slomo) from comment #6) > When are buffers timestamped in the source? Just when they arrive in ahcsrc > or by Android before that already? ahcsrc calculates timestamp when the buffer is arrived. Android just sent bytes stream to ahcsrc currently.
The the latency given in the latency query seems correct (the max latency should actually be higher but doesn't matter here), and your problem is elsewhere. The timestamping logic is wrong or downstream is introducing unreported latency.
The problem is probably that we accumulate buffers in paused, instead of destroying it. So when we go back to playing, we effectively are 5 frames late.
Good idea :)
Created attachment 355059 [details] [review] Drop buffers in paused Dropping buffers when it's not playing.
Review of attachment 355059 [details] [review]: ::: sys/androidmedia/gstahcsrc.c @@ +2118,3 @@ } + if (GST_STATE (self) != GST_STATE_PLAYING) You'll need the state lock to check that. Though, isn't there a cleaner way ?
(In reply to Nicolas Dufresne (stormer) from comment #12) > Review of attachment 355059 [details] [review] [review]: > Though, isn't there a cleaner way ? I thought 'gst_ahc_src_on_preview_frame' is a right place to avoid surplus memory allocation. The other possible approaches are; - dropping buffers when transiting from PAUSED to PLAYING - or, checking buffer timestamp when 'basesrc->create' is called. If I am wrong, any suggestion?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/357.