On 5/29/24 14:24, Michael Grzeschik wrote: > On Tue, May 28, 2024 at 05:33:46PM -0700, Avichal Rakesh wrote: >> >> >> On 5/28/24 15:43, Michael Grzeschik wrote: >>> On Tue, May 28, 2024 at 02:27:34PM -0700, Avichal Rakesh wrote: >>>> >>>> >>>> On 5/28/24 13:22, Michael Grzeschik wrote: >>>>> On Tue, May 28, 2024 at 10:30:30AM -0700, Avichal Rakesh wrote: >>>>>> >>>>>> >>>>>> On 5/22/24 10:37, Michael Grzeschik wrote: >>>>>>> On Wed, May 22, 2024 at 05:17:02PM +0000, Thinh Nguyen wrote: >>>>> One option to be totally sure would be to resend the sentinel request to >>>>> be properly transmitted before starting the next frame. This resend >>>>> polling would probably include some extra zero-length requests. But also >>>>> if this resend keeps failing for n times, the driver should doubt there >>>>> is anything sane going on with the USB connection and bail out somehow. >>>>> >>>>> Since we try to tackle case (1) to avoid transmit errors and also avoid >>>>> creating late enqueued requests in the running isoc transfer, the over >>>>> all chance to trigger missed transfers should be minimal. >>>> >>>> Gotcha. It seems like the UVC gadget driver implicitly assumes that EOF >>>> flag will be used although the userspace application can technically >>>> make it optional. >>> >>> That is not all. The additional UVC_STREAM_ERR tag on the sentinel >>> request can be set optional by the host driver. But by spec the >>> userspace application has to drop the frame when the flag was set. >> >> Looking at the UVC specs, the ERR bit doesn't seem to refer to actual >> transmission error, only errors in frame generation (Section 4.3.1.7 >> of UVC 1.5 Class Specification). Maybe "data discontinuity" can be >> used but the examples given are bad media, and encoder issues, which >> suggests errors at higher level than the wire. > > Oh! That is a new perspective I did not consider. > > With the definition of UVC_STREAM_ERR by spec, the uvc_video driver > would in no case set this header bit for the current frame on its own? > Is that correct? It would indeed seem so. The way gadget driver is architected makes is impossible for the userspace application to notify the host of any errors. > >>> With my proposal this flag will be set, whenever we find out that >>> the currently transferred frame was erroneous. >>> >>>> Summarizing some of the discussions above: >>>> 1. UVC gadget driver should _not_ rely on the usb controller to >>>> enqueue 0-length requests on UVC gadget drivers behalf; >>>> 2. However keeping up the backpressure to the controller means the >>>> EOF request will be delayed behind all the zero-length requests. >>> >>> Exactly, this is why we have to somehow finetune the timedelay between >>> requests that trigger interrupts. And also monitor the amount of >>> requests currently enqueued in the hw ringbuffer. So that our drivers >>> enqueue dequeue mechanism is virtually adding only the minimum amount >>> of necessary zero-length requests in the hardware. This should be >>> possible. >>> >>> I am currently thinking through the remaining steps the pump worker has >>> to do on each wakeup to maintain the minimum threshold while waiting >>> with submitting requests that contain actual image payload. >>> >>>> Out of curiosity: What is wrong with letting the host rely on >>>> FID alone? Decoding the jpeg payload _should_ fail if any of the >>>> usb_requests containing the payload failed to transmit. >>> >>> This is not totally true. We saw partially rendered jpeg frames on the >>> host stream. How the host behaves with broken data is totally undefined >>> if the typical uvc flags EOF/ERR are not used as specified. Then think >>> about uncompressed formats. So relying on the transferred image format >>> to solve our problems is just as wrong as relying on the gadgets >>> hardware behavior. >> >> Do you know if the partially rendered frames were valid JPEGs, or >> if the host was simply making a best effort at displaying a broken >> JPEG? Perhaps the fix should go to the host instead? > > I can fully reproduce this with linux and windows hosts. For linux > machines I saw that the host was taking the FID change as a marker > to see the previous frame as ready and just rendered what got through. > This did not lead to garbage but only to partially displayed frames > with jpeg macroblock alignment. I was aware of linux doing so, but I only ever saw this behavior on Windows if there were a lot of invalid frames back to back. I am not super familiar with the guarantees of JPEG, but I suppose it is possible to have a "valid" JPEG with some middle blocks missing as long the EOI bits make it through? I am not sure how we go about solving that. > >> Following is my opinion, feel free to disagree (and correct me if >> something is factually incorrect): >> >> The fundamental issue here is that ISOC doesn't guarantee >> delivery of usb_requests or even basic data consistency upon delivery. >> So the gadget driver has no way to know the state of transmitted data. >> The gadget driver is notified of underruns but not of any other issues, >> and ideally we should never have an underrun if the zero-length >> backpressure is working as intended. >> >> So, UVC gadget driver can reduce the number of errors, but it'll never be >> able to guarantee that the data transmitted to the host isn't somehow >> corrupted or missing unless a more reliable mode of transmission >> (bulk, for example) is used. >> >> All of this to say: The host absolutely needs to be able to handle >> all sorts of invalid and broken payloads. How the host handles it >> might be undefined, but the host can never rely on perfect knowledge >> about the transmission state. In cases like these, where the underlying >> transport is unreliable, the burden of enforcing consistency moves up >> a layer, i.e. to the encoded payload in this case. So it is perfectly >> fine for the host to rely on the encoding to determine if the payload >> is corrupt and handle it accordingly. > > Right. > >> As for uncompressed format, you're correct that subtle corruptions >> may not be caught, but outright missing usb_requests can be easily >> checked by simply looking at the number of bytes in the payload. YUV >> frames are all of the same (predetermined) size for a given resolution. > > That was also my thought about five minutes after I did send you the > previous mail. So sure, this is no real issue for the host. > >> So my recommendation is the following: >> 1. Fix the bandwidth problem by splitting the encoded video frame >> into more usb_requests (as your patch already does) making sure >> there are enough free usb_request to encode the video frame in >> one burst so we don't accidentally inflate the transmission >> duration of a video frame by sneaking in zero-length requests in >> the middle. > > Ack. This should already solve a lot of issues. > > For this I would still suggest to move the usb_ep_queue to be done in > the pump worker again. Its a bit back and forth, but IMHO its worth the > extra mile since only this way we would respect the dwc3 interrupt > threads assumption to run *very* short. The main reason for queuing the requests from the complete handler was to have a single point of usb_ep_queue call, which made reasoning through the locking simpler. But if you find a way to do so from the video_pump thread without making the locking a nightmare, then go for it! > >> 2. Unless there is an unusually high rate of transmission failures >> when using the UVC gadget driver, it might be worth fixing the >> host side driver to handle broken frames better instead (assuming >> host is linux as well). > > Agreed, but this needs a separate scoped undestanding of the host side > behaviour over all layers. Agreed! > >> 2. Tighten up the error checking in UVC gadget driver -- We drop the >> current frame whenever an EXDEV happens which is wrong. We should >> only be dropping the current frame if the EXDEV corresponds to the >> frame currently being encoded. > > What do you mean by drop? > > I would suggest to immediatly switch the uvc_buffer that is being > enqueued and start queueing prepared requests from the next buffers prep > list. As suggested, the idea is to have per uvc_buffer prep_list > requests which would make this task easy. Currently, if uvc gadget driver receives an EXDEV complete callback all it does is set the UVC_QUEUE_DROP_INCOMPLETE flag. So let's say that we receive an EXDEV for a usb_request containing data for video frame N. With how video_pump is currently configured, chances are that all usb_requests containing data for video frame N has already been queued to the controller. When the next video frame (N+1) comes in, video_pump's encode methods will look at the UVC_QUEUE_DROP_INCOMPLETE flag and incorrectly determine that "current" frame needs to be dropped, and stop encoding video frame N+1 even though the error was for video frame N. So the encode methods incorrectly drop video frame N+1 which isn't needed. The encode methods should only be dropping the video frame if we received an EXDEV for a usb_request for the video frame currently being encoded. I hope that makes sense! - Avi.
