1. On Wed, 2020-03-25 at 16:28 +0100, Hans Verkuil wrote: > On 3/25/20 3:02 PM, Nicolas Dufresne wrote: > > Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit : > > > On 3/18/20 2:21 PM, Ezequiel Garcia wrote: > > > > Let the core sort out the nuances of returning buffers > > > > to userspace, by using the v4l2_m2m_buf_done_and_job_finish > > > > helper. > > > > > > > > This change also removes usage of buffer sequence fields, > > > > which shouldn't have any meaning for stateless decoders. > > > > > > Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance? > > > Also, while I agree that it is not terribly useful, it doesn't hurt, does it? > > > > > > And the V4L2 spec makes no exception for stateless codecs with respect to the > > > sequence field. > > > > > > Nacked-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> > > > > The spec also does not say what it means either. As an example, you > > have spec for ALTERNATE interlacing mode that changes the meaning of > > the sequence, but not for alternate H264 fields (which cannot be part > > of the format, since this changes often). We also don't have spec for > > the the sequence behaviour while using HOLD features. > > I hate it that the spec changes the sequence meaning for FIELD_ALTERNATE, > I always thought that that made drivers unnecessarily complicated. Unfortunately, > this is something we inherited. > > Currently the spec says for sequence: > > "Set by the driver, counting the frames (not fields!) in sequence. This field is set > for both input and output devices." > > The only thing missing here is that it should say that for compressed formats this > counts the buffers, since one buffer with compressed data may not have a one-to-one > mapping with frames. > > This description for 'sequence' was never updated when compressed data formats were > added, so it is a bit out of date. > > > I'm worried we are falling into the test driven trap, were people > > implement features to satisfy a test, while the added complexity don't > > really make sense. Shouldn't we change our approach and opt-out > > features for new type of HW, so that we can keep the drivers code > > saner? > > Why wasn't the existing code in this patch sane? Sure, we can change the spec, but > then 1) all existing drivers need to be updated as well, and 2) v4l2-compliance needs > to be changed to test specifically for this class of drivers and ensure that for those > the sequence field is set to 0. Not to mention introducing an exception in the uAPI > where the sequence field suddenly isn't used anymore. > > Frankly, I would prefer that the whole sequence handling is moved to videobuf2-v4l2.c. > It really doesn't belong in drivers, with the exception of incrementing the sequence > counter in case of dropped frames. > > I think I suggested it when vb2 was being designed, but at the time the preference > was to keep it in the driver. Long time ago, though. > Do you think we could try to move this to the core? I might be able find some time to try that. > And another reason why I want to keep it: I find it actually useful to see a running > counter: it helps keeping track of how many buffers you've processed since you started > streaming. > +1 > Finally, the removal of the sequence counter simply does not belong in this patch. > Agreed, no complaints on my side. I am actually happy about this feedback. Thanks, Ezequiel
