<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
***: KitsuWhooa has quit IRC (*.net *.split)
<br> tensa has quit IRC (*.net *.split)
<br> broonie has quit IRC (*.net *.split)
<br> Marex has quit IRC (*.net *.split)
<br> evalenti has quit IRC (*.net *.split)
<br> darvon has quit IRC (*.net *.split)
<br> mripard has quit IRC (*.net *.split)
<br> andrey_utkin has quit IRC (*.net *.split)
<br> r0kc4t has quit IRC (*.net *.split)
<br> unidan has quit IRC (*.net *.split)
<br> mjourdan has quit IRC (*.net *.split)
<br> juvenal has quit IRC (*.net *.split)
<br> orwell.freenode.net sets mode: +o ChanServ
<br> paul374 has left
hverkuil: <u>jernej</u>: you are right, job_finish should always be called.
jmondi: <u>ribalda</u>: thanks, sorry I haven't noticed v2 was out already... looking forward to your v3 then
mfelsch: <u>hverkuil</u>: ping
hverkuil: <u>mfelsch</u>: pong
mfelsch: <u>hverkuil</u>: I've seen that my fwnode patch had a leading semicolon
<br> <u>hverkuil</u>: Should I send another version or just a fixup patch?
hverkuil: I'd post a v8.1 of just that patch.
mfelsch: Thanks
<br> <u>hverkuil</u>: Should that patch be able to be squashed?
hverkuil: No, it just replaces the old v8 patch with the fixed new one. Or are later patches dependent on that change as well?
mfelsch: No, now I got you. Thanks
sailus: <u>mripard</u>: Ping?
paulk-leonov: <u>hverkuil</u>: Could I add the HEVC decode mode and start code as a separate patch for HEVC since it won't really be supported at this point?
<br> or two patches
<br> to clarify this in the commit message
<br> <u>hverkuil</u>: also I just realized that the values for the start code and decoding mode controls are specified as enums for h264
<br> I thought enums are not acceptable for uapi since there are no guarantees about their values
hverkuil: The control uses s32 as the value.
paulk-leonov: to be honest really I don't see the point of adding these controls for hevc at this point since it's not supported
<br> <u>hverkuil</u>: right but the compiler can give e.g. V4L2_MPEG_VIDEO_H264_DECODE_MODE_SLICE_BASED any value, or am I missing something?
hverkuil: No, it can't. It can choose the type (char, int, short, whatever), but the values are fixed (starting at 0).
paulk-leonov: proper per-slice/per-frame decoding will certainly require reworking the controls for hevc anyway
<br> <u>hverkuil</u>: oh okay, my bad
hverkuil: But the hevc driver is currently hardcoded to use a certain decoding mode and start code. So that should be exposed.
paulk-leonov: <u>hverkuil</u>: it's none of the two that are exposed
<br> like our h264 implementation
<br> it's a weird mix between the two
hverkuil: I'll get back to you later, have to finish something else first.
paulk-leonov: <u>hverkuil</u>: sure
doublef: Hi all, is this a proper place for a UVC question? I'm having doubts about the bytesused == dwMaxVideoFrameSize equality check in uvc_video_validate_buffer().
hverkuil: paulk-leonov: so post support for those controls in separate patches, and we can deal with them later. With a bit of luck the work that jernej is doing for h264 can be ported for hevc as well without too many problems.
paulk-leonov: <u>hverkuil</u>: great, thanks :)
hverkuil: and it's certainly much easier for jernej to work on this if basic hevc support is merged.
pH5: <u>doublef</u>: "dwMaxVideoFrameSize - For frame-based formats, this field indicates the maximum size of a single video frame." - shouldn't that be exactly the same as the actual frame size for uncompressed formats?
paulk-leonov: <u>hverkuil</u>: right that was sort of my underlying point, that it's easier to have a half-broken base merged
<br> and then move forward on that
doublef: <u>pH5</u>: I understand it should, but does it *have to*? it's a maximum size, after all.
<br> <u>pH5</u>: some more details on this in https://www.spinics.net/lists/linux-media/msg156105.html
pH5: <u>doublef</u>: hm, I'd consider it a hardware bug if the camera lies about the max frame size, but as long as dwMaxVideoFrameSize is too large I'd argue uvcvideo should accept the frames anyway.
<br> <u>doublef</u>: possibly locked behind a device quirk?
doublef: <u>pH5</u>: check out the link --- it's not just the camera lying:)
<br> <u>pH5</u>: a device quirk is a possible solution, but I'm not really sure if that's the only device affected.
pH5: I suppose the camera will never send more than 3110400 bytes per frame for the NV12 format?
<br> I'd expect that to be the dwMaxVideoFrameSize then.
<br> Why should the receiver be ready to receive 4147200 bytes?
doublef: dwMaxVideoFrameSize announched by the camera is 12 bytes larger (IDK why; somehow some alignment-related thoughts come to mind). But it's overwritten by dwMaxVideoFrameBufferSize by the kernel.
<br> Even if the camera sent the exactly correct value for dwMaxVideoFrameSize, it would have been overwritten.
pH5: Hm. In UVC 1.5 dwMaxVideoFrameSize is deprecated. Maybe uvc_fixup_video_ctrl should use the max(framesize,framebuffersize).
<br> <u>doublef</u>: I'd relax the check to !(bytesused <= dwMaxVideoFrameSize) and maybe instead check that bytesused is equal to v4l2 sizeimage.
doublef: <u>pH5</u>: the <= check also seems reasonable and justified by the spec to me. I'll look into sizeimage but I haven't found a pint where it's different from dwMaxVideoFrameSize.
<br> *point
pH5: s/In UVC 1.5 dwMaxVideoFrameSize is deprecated./In UVC 1.5 dwMaxVideoFrameBufferSize is deprecated./
<br> it still must be set for backwards compatibility though
doublef: As far as I understand the intent of uvc_video_validate_buffer() was to weed out obviously broken/incomplete frames. However, shouldn't USB handle that? it has CRCs all over the place...
pH5: I think you can end up with short frames if either isochronous payloads are lost on the wire, or if the frame packager in the device just doesn't receive sensor data quickly enough.
<br> In both cases you get correct data, just less than expected.
pinchartl: <u>doublef</u>: why is dwMaxVideoFrameSize set to 4147200 for 1920x1080 NV12 in the first place ?
doublef: Hmm. Does the first issue affect bulk transfers?
pinchartl: as for if the check could be removed, there are cameras that drop packets, without the host notificing. the size check is all we have to catch that
doublef: <u>pinchartl</u>: it's set to 3110412, but later overwritten by the kernel to be dwMaxVideoFrameBufferSize (why that is 4147200, I have no idea).
pinchartl: where is it overwritten ?
doublef: https://github.com/torvalds/linux/blob/95f5cbff90b9e4324839a5c28ee3153a3c9921a5/drivers/media/usb/uvc/uvc_video.c#L119
<br> doesn't matter as 3110412 isn't exactly 3110400 anyway.
pinchartl: that only happens if dwMaxVideoFrameSize == 0
<br> ah, pre UVC 1.1
doublef: that's why I mentioned double-checking the logic of that condition in my emails.
pH5: <u>pinchartl</u>: or unconditionally, for uncompressed formats
doublef: It happens for all uncompressed formats, period.
pinchartl: indeed
<br> I missed that
-: pinchartl needs to wake up :-)
pH5: <u>doublef</u>: I don't know about bulk transfers, I don't have any bulk cameras (left). I've seen losses happen with isoc devices where I can control the sensor though.
pinchartl: let me check the pre-git history
pH5: uvc archaeology :)
ezequielg: paulk-leonov: i must admit, i am not sure i understand your comment about cedrus h264 expectations.
paulk-leonov: <u>ezequielg</u>: it was mostly to say that currently, the output buffer expectations do not match what the new start code/decoding mode expect
ezequielg: can you explain why?
paulk-leonov: <u>ezequielg</u>: because it will only process a single slice
<br> mhh wait
<br> I must be confused about it as well
jernej: <u>hverkuil</u>: paulk-leonov: I have working code for h264 multi-slice frames. Hopefully I'll be able to send patches in the evening or tomorrow.
ezequielg: <u>jernej</u>: \o/ great.
paulk-leonov: <u>ezequielg</u>: nevermind, I guess it's true for h264
<br> it's for mpeg-2 that we do slice gathering
ezequielg: paulk-leonov: fwiw, we went to great extents to make sure the proposal fits cedrus and hantro.
paulk-leonov: <u>jernej</u>: nice work!
ezequielg: so i was surprised to hear you thought it was broken.
<br> since it's what we discussed and agreed.
hverkuil: paulk-leonov: what is true for h264? (I'm now terminally confused)
paulk-leonov: I think I mixed up the situations on mpeg-2 and h264
<br> on mpeg-2 we currently do slice concatenation and I think it was intended to be the same on h264, though in practice it looks like it only reads the first slice, which is what the new control specifies
<br> sorry about the noise, I don't have the right context in mind
jernej: <u>hverkuil</u>: I hope you don't mind if I fix example in the documentation and send those patches along with cedrus changes?
hverkuil: <u>jernej</u>: sure, go ahead! That's what happens when you 'dry program' :-)
paulk-leonov: <u>jernej</u>: one thing I'd be interested in next is the ability to have multiple slices per output buffer
<br> but we should discuss this at a later point :)
hverkuil: <u>ezequielg</u>: does the hantro support slices for MPEG2?
ezequielg: <u>hverkuil</u>: that's a question i'm not in a position to answer now :-)
hverkuil: The only real metadata that a MPEG2 slice has is the quantiser_scale_code. All other metadata is per-frame.
<br> But that's not really reflected in the MPEG2 stateless API.
<br> If the decoding mode is per-frame (so all slices in one buffer), then we can't set the quantiser_scale_code (or start_byte_offset which you would need for consistency with h264) for each slice.
<br> It would work if it is one slice per buffer, but the available decoders don't support that either.
<br> <u>ndufresne</u>: do you know more about the use of slices in MPEG2?
ezequielg: <u>hverkuil</u>: we are starting to see more pix-fmt modifiers. maybe the controls should be explicitly named _modifier?
<br> (or am i bikeshedding?)
doublef: pinchartl pH5: thanks a lot for the discussion, I need to go now...
hverkuil: <u>ezequielg</u>: I think we discussed that at some point. It's debatable whether it is a pixel modifier or just a way to report decoder limitations.
<br> If I remember I leaned towards the latter.
jernej: paulk-leonov: with that you mean frame mode like hantro, where you queue all slices in single buffer?
paulk-leonov: <u>jernej</u>: I was thinking something in-between
<br> with an arbitrary number of slices
jernej: Ah, that should be doable too
paulk-leonov: the problem I see is that one slice per output buffer will probably result in under-using the buffers
<br> but maybe it's not that big a deal
ezequielg: <u>jernej</u>: performance wise, implementing frame-mode in cedrus would be the best, right?
pinchartl: <u>doublef</u>: pH5: http://paste.debian.net/1096977/
jernej: <u>ezequielg</u>: probably, but that also depends on efficiency of userspace app implementation. With current ffmpeg codec, frame mode is certainly more efficient, but slice mode could be improved
<br> I think there is still wait on every slice job finish, but best would be to wait only on last one.
wens: seems neither hantro nor cedrus looks at .num_slices though
ezequielg: <u>wens</u>: because cedrus assumes 1 slice, and the hantro hardware is able to parse the buffer itself.
wens: <u>ezequielg</u>: so the app could pass multiple slices, up to one frame, to hantro
<br> sounds like libva-v4l2-request should work without modifications
ezequielg: <u>wens</u>: right - in fact, our ffmpeg implementatio supports that. multi-slice videos work.
wens: AFAIK libva-v4l2-request does one slice at a time, being VA-API based