On 09/11/2018 15:41, Philipp Zabel wrote:
> On Wed, 2018-11-07 at 22:25 +0200, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> On Wednesday, 7 November 2018 16:30:46 EET Kieran Bingham wrote:
>>> On 06/11/2018 23:13, Laurent Pinchart wrote:
>>>> On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
>>>>> From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>>>>>
>>>>> We have both uvc_init_video() and uvc_video_init() calls which can be
>>>>> quite confusing to determine the process for each. Now that video
>>>>> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
>>>>> adapt these calls to suit the new flow.
>>>>>
>>>>> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
>>>>> uvc_video_stop().
>>>>
>>>> I agree that these functions are badly named and should be renamed. We are
>>>> however entering the nitpicking territory :-) The two functions do more
>>>> that starting and stopping, they also allocate and free URBs and the
>>>> associated buffers. It could also be argued that they don't actually
>>>> start and stop anything, as beyond URB management, they just queue the
>>>> URBs initially and kill them. I thus wonder if we could come up with
>>>> better names.
>>>
>>> Well the act of killing (poisoning now) the URBs will certainly stop the
>>> stream, but I guess submitting the URBs isn't necessarily the key act to
>>> starting the stream.
>>>
>>> I believe that needs the interface to be set correctly, and the buffers
>>> to be available?
>>>
>>> Although - I've just double-checked uvc_{video_start,init_video}() and
>>> that is indeed what it does?
>>>
>>> - start stats
>>> - Initialise endpoints
>>> - Perform allocations
>>> - Submit URBs
>>>
>>> Am I missing something? Is there another step that is pivotal to
>>> starting the USB packet/urb stream flow after this point ?
>>>
>>>
>>> Is it not true that the USB stack will start processing data at
>>> submitting URB completion callbacks after the end of uvc_video_start();
>>> and will no longer process data at the end of uvc_video_stop() (and thus
>>> no more completion callbacks)?
>>>
>>> (That's a real question to verify my interpretation)
>>>
>>> To me - these functions feel like the real 'start' and 'stop' components
>>> of the data stream - hence my choice in naming.
>>
>> The other part of the start operation is committing the streaming parameters
>> (see uvc_video_start_streaming()). For the stop operation it's issuing a
>> SET_INTERFACE or CLEAR_FEATURE(HALT) request (see uvc_video_stop_streaming()).
>>
>>> Is your concern that you would like the functions to be more descriptive
>>> over their other actions such as? :
>>>
>>> uvc_video_initialise_start()
>>> uvc_video_allocate_init_start()
>>>
>>> Or something else? (I don't think those two are good names though)
>>
>> Probably something else :-) A possibly equally bad proposal would be
>> uvc_video_start_transfer() and uvc_video_stop_transfer().
>
> I think this is still better than what we have now.
>
> At least it contains "transfer" to make it clear it deals with the
> isoc/bulk transfer setup/teardown part of streaming, not actually
> starting or stopping the device streaming.
Ok - uvc_video_start_transfer() and uvc_video_stop_transfer() it shall
be then :)
v6 with fixed stream object / workqueue lifetime and updates to the last
two patches coming ... soon :)
--
Kieran
>
> regards
> Philipp
>
--
Regards
--
Kieran
