<!-- 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>
ndufresne: Kwiboo, interestingly, you also set the index in ffmpeg / v4l2_request_dequeue_buffer
Kwiboo: ndufresne, I looked at the VA driver v4l2 code for inspiration and to learn about v4l2 api, so if you see same mistake in both VA and ffmpeg ... (I knew next to nothing about v4l2 api before I created the ffmpeg hwaccel)
<br> regarding if a frame could reference itself, technically it is a field that can reference the other field located in same capture buffer
<br> ndufresne, bbrezillon, paulk-leonov: I made a discovery during yesterdays + this mornings testing of rk h264, it actually need the START_CODE_E bit set, or else only some of the videos will decode others will result in complete green screen
<br> my big buck bunny samples will play without start_code_e, but almost all my other h264 samples result in green screen without start_code_e set and buffer pre-padded with the 3 byte start code
bbrezillon: <u>Kwiboo</u>: I had only tested it on 2 samples
<br> I just tested it on one with multiple 2 slices per frame, and half of the screen is green
<br> s/multiple//
<br> I'd expect it to be a problem when we pack slices and do 1 decode operation per frame, but it seems to also impact per-slice decoding
Kwiboo: bbrezillon, yep, I am seeing similar issues we have used to see on cedrus (related to reference pictures), so I am trying to sort some of them out
ndufresne: bbrezillon, if we remove the start-code, we need to set the IP in slice mode (1 interrupt per slice) and to make sure one output buffer contains exactly one slice
<br> Kwiboo, that's interesting, so we really need to assign timestamps early, otherwise if you do being/field1/field2/end, you'd endup with the the second field not finding field1, because that buffer was not yet "produce"
<br> hverkuil, ^ we need timestamp to be assigned before a frame is complete
<br> Kwiboo, jernej: I tried many things to get the codec to work with simply bootlin branch on github, I really don't know how that branch was tested, but cannot make it work with any non-trivial streams, anyway, I'll base on 5.1 now and apply LibreELEC patches, if that does not work, I'll start thinking that H3/Tritium haven't been tested
jernej: <u>ndufresne</u>: I don't have Tritium board, although board model shouldn't matter here
<br> I plan to merge my LE patches upstream, but that can't happen until H264 series is merged
ndufresne: All I know, is that I haven't found any combination of the bootlin branch and some userspace that would produce correct images
jernej: ffmpeg request api codec was only really tested with LE patches, not vanilla patch series
ndufresne: ok, so that's what I'll move to now
jernej: however, remove top/bottom reference hack from ffmpeg, that should improve your tests considerably
<br> btw, I just found out that libvdpau-sunxi correctly decodes all problematic videos
<br> so I have good base for comparing register values
ndufresne: I thought these bits would always be 0 ?
<br> ok, that must explain, since I assume that as long as it was not interlaced, the index would not be affected by this
jernej: not sure, but try without setting them
<br> btw, do we have any satisfactory solution for these two flags?
ndufresne: not yet I think
<br> I was actually wonder what the second bit is used for in the LE patches
<br> since you can have one to mean TOP/BOTTOM, so the other is for ?
jernej: I think one is for top another for bottom and both combined mean frame, but I'm not 100% sure
ndufresne: ok, but references are top or bottom, not both
<br> well, top/bottom/frame
jernej: as comment says, it's a hack, so not everything may make sense or it's needed :)
<br> only bottom flag is used, second bit is not
ndufresne: I think we need to know a little on how the IP is programmed
<br> and if you can know that the streams is progressive ...
<br> jernej, ok, so that match, on RK/mpp they use the same thing btw, 5 bit for the index, 1 bit for bottom, and 1 bit for the right view
<br> now my issue with that is because it's very limited for views
<br> I know views are mostly use for stereoscopic, but I'm wondering if it's not more generic in practice
jernej: I'm not even sure how this is supported in AW VPU
<br> I never tried stereoscopic stream
ndufresne: ok, I checked and there is two multiview profile, Multiview High Profile and Stereo High Profile
<br> the RK only support the second, but the first can have an arbitrary amount of views
<br> jernej, in general, stereo profile get decoded into a single surface, with a decoder specific layout, side-by-side seems quite common
<br> there is even some older files that are no-mvc, but have this side-by-side layout
jernej: so there is no need for special register settings for stereo profile?
<br> anyway, I leave stereo streams for a later time
ndufresne: well, in the references list, you need to specify the view ID
<br> now I'm pretty sure you don't mix interlace and stereo
<br> since if you did, then the reference list size would be 16 * 2 * num-views
<br> my main worry is the reference list, as I don't know how the view works, but if they do create a multiplier on the list size, like interlaced do, then we have a problem
<br> an option is to make H264_MVC_SLICE later on, and just create a new set of controls
jernej: or just an extension control with extra info
ndufresne: we'll need to expose this in the list of H.264 profile, so we could document per profile the extra controls to use, good point
<br> At the same time, for stereo, all that RK needs is 1 bit, since it's stereo
<br> jernej, do what about having two arrays, e.g. l0_index l0_flags ?
<br> this way for simple case, we'd have 8 bit of flags (or more) to extend it, and for the complex case, we'd introduce a new control
<br> interesting, VDPAU stopped before H.264 MVC was completed
jernej: which vdpau?
<br> yeah, two arrays work for me. I think I saw that already somewhere.
ndufresne: is it me of in VDPAU, the modified list and the parsing of the modificaiton is left to the driver ?
<br> VDPAU has some interesting bit, the start-code is mandatory, but you can pass it in a seperate buffer
jernej: you noticed correctly, in VDPAU a lot of things has to be done by driver if it's not supported in HW
ndufresne: quick question, what's the base for LE patchset ?
jernej: 5.1
ndufresne: I just tried but "media: v4l: Add definitions for the HEVC slice format and controls" failed
jernej: <u>ndufresne</u>: check libavcodec/dxva2_h264.c for another interface. Here bottom reference flag is added in same byte as index
<br> did you apply 0002-fixes-from-5.2.patch?
<br> maybe "fixes" should be changed to "backport"
ndufresne: I was hoping this would do, "git am -3 ../LibreELEC.tv/./projects/Allwinner/patches/linux/*.patch"
jernej: sadly no, because one or two patches don't have git form
<br> e.g. no header
<br> you have to use "patch -p1 < /path/to/file.patch" on them
ndufresne: arg
<br> I have always hated this process of having everyone assembling the same git branch, and hoping for no human error
<br> ok, will go through that another day
jernej: I'll update those patches as soon as something is merged in linux-media branch
<br> after that it will be possible to use git am on them
bbrezillon: <u>ndufresne</u>: will look at the regs to see how this "slice-decoding" mode can be enabled
<br> <u>Kwiboo</u>: if you have a sample that fails, I can have a look on Monday
Kwiboo: bbrezillon, ndufresne: is there any reason for the update_dpb in rk driver? some of my progressive samples got improved by removing the dpb reordering, most notable was http://jernej.libreelec.tv/videos/h264/test.mkv
<br> field references is still an issue, but I hope to have it working soon, will push some reworked code and generate the code for vpu2 (rk3328/rk3399) later
bbrezillon: <u>Kwiboo</u>: tfiga told me it was needed
<br> testing without this "re-use existing entries" logic was on my TODO list
<br> <u>Kwiboo</u>: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4d7cb46539a93bb6acc802f5a46acddb5aaab378
<br> this being said, this should have no impact on the output (unless something is buggy in the code)
Kwiboo: it could have been me refactoring and doing something wrong, but I hade the opposite effect, re-ordering caused issues and removing it solved my issue
bbrezillon: <u>Kwiboo</u>: so you just copy the passed DPB instead?
<br> (or simply remove the cached version attached to the context)
<br> ?
Kwiboo: yep, in the code I had running a few minutes ago I looped and copied dec_param->dpb to h264_ctx->dpb, but going to test with removing h264_ctx->dpb completly now
bbrezillon: ok
<br> <u>Kwiboo</u>: IIRC, MPP is also doing that
<br> (re-using existing entries instead of using the new DPB directly)
Kwiboo: <u>bbrezillon</u>: https://github.com/Kwiboo/linux-rockchip/commit/a78e6b4b01d43be0f7eeb664f2842a6a0f23ecf4 is the current state of reworked code (everything squashed into one commit :/), I mainly moved code around to make it possible to use my rockchip-vpu-regtool in order to generate code for vpu2
ndufresne: Kwiboo, just so I read correctly, this is about relying on the dpb entries order passed from userspace instead of reordering it in the kernel ? I generally preferred not to impose any order on the references, but there is interesting info here to fix the kernel reorder (assuming my reading is correct)
bbrezillon: <u>ndufresne</u>: it's not about the re-ordering
ndufresne: oh, ok
bbrezillon: just just logic that tries to re-use previous DPB entries
ndufresne: ah, ok
bbrezillon: instead of trashing the old DPB and using the old one directly
<br> I asked tfiga what was the reason for this extra step a few weeks back
<br> and he pointed me to https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4d7cb46539a93bb6acc802f5a46acddb5aaab378
ndufresne: ok, but the patch commit message is all about ordering, so that's odd
<br> bbrezillon, and with your code, you could really receive random order, as long as everything is there it should work
bbrezillon: yep
<br> actually, Kwiboo still relies on in-kernel ref list creation
<br> it's just that the step that tries to re-use old DPB entries (those from the previous decoding request) is bypassed
<br> <u>Kwiboo</u>: thanks
ndufresne: ok, well, I'm all in favour of less code, have to leave, see you
Kwiboo: it could be that ffmpeg sends the dpb in a order that hw is expecting, but that seems unlikely because it pushes new reference frames at position 0 instead of end of list
<br> I updated my rockchip-vpu-regtool with the missing hw regs in https://github.com/Kwiboo/rockchip-vpu-regtool/commit/d5d8d12900dea70d39981b5b10fa68ce0d593b94
bbrezillon: I added traces to debug the reflist creation code, so I'm sure it does re-order things :P
Kwiboo: still having some issues with field encoded videos, but I think it is mainly because it uses wrong poc, code relies on buf->field, but buf->field is not set by ffmpeg
bbrezillon: yep, if buf->field is incorrect it can't work
<br> <u>Kwiboo</u>: you'll also have to fix dpb_valid/dpb_longterm and set the 2 LSB of REF_PIC() to the appropriate value
<br> <u>Kwiboo</u>: hm, so it looks like the order of refs matters (even if the ref list points to the updated entry)
<br> I tried swapping dpb[0] and dpb[1] (I'm using the DPB passed by ffmpeg in my tests), and I see artifacts
Kwiboo: I just tried sending dpb in reverse order and nothing happend, also running with start_code_e=0 seems to work now
<br> maybe some other change helped solve the issue
<br> I have not been able to get slice decoding working, I have to send all slices in one buffer and decode the whole frame
bbrezillon: slice decoding was working for me
<br> at least I think it was
<br> I mean mutiple slices per frame
<br> though I only tested with 2 slices
<br> <u>Kwiboo</u>: what else did you change between your 2 runs?
Kwiboo: one thing I also changed was that _nbr regs gets set to 0 if dpb flags is not active
<br> I am also using these ffmpeg changes: https://github.com/Kwiboo/FFmpeg/commit/59e924ba81087a30efac07f1296f11fabe8b4c22
bbrezillon: <u>Kwiboo</u>: you don't seem to use the ff_v4l2_request_decode_slice() function I added
Kwiboo: ahh, will test that later :)
bbrezillon: you need to trigger a decode per slice if you want per-slice decoding to work :P
<br> looks like you only trigger a decode operation on ->end_frame()
Kwiboo: with the code commited yes, but the code I tested was triggering a v4l2_request_h264_end_frame in similar way that works for cedrus, i.e. calling v4l2_request_h264_end_frame
<br> I did not want to pick all the new _HOLD patches yet :)
bbrezillon: oh, ok
<br> then I guess it's fine
<br> setting _NBR to 0 does not solve the issue :-/
flok: any idea why VIDIOC_DQBUF always hangs with my pac207 cameras?
<br> v4l2-ctl --all -d /dev/video0 shows "streaming" in the capabilities, afaik that is mmap()ed access, right?