mchehab: you mean pull requests late at -rc cycle? If so, yes I will not. The only exception is the regression fix https://patchwork.linuxtv.org/patch/56338/ but I can see it in media/fixes
bbrezillon: paulk-leonov: svarbanov: ezequielg: gnurou: tfiga: mripard_: mjourdan: mtretter: ndufresne: I have update v4l-utils with the latest codec compliance tests.
No need to use my old vicodec branch in my own v4l-utils repo anymore.
I am sure there will be issues when attempting to use the compliance tests with real hardware, just let me know.
One thing I would like to have are test bitstreams for the various codecs, including some with a midstream resolution change (preferably going from a lower to a higher resolution).
Note: the compliance tests are for stateful codecs only, not for stateless.
hverkuil: Thanks for the update! do you need help for these bitstreams tests
hverkuil: also I interested of bbrezillon _ext_ ioctls patchset and more specifically to _modifiers_, because I need to support compressed raw pixel formats. Do we have some idea how _modifiers_ will function
svarbanov: the plan was to mimic what's done in DRM
so you'd have a 4CC pixel format and a modifier applied on top, like NV12+Tiled
hverkuil: nice, thanks :)
bbrezillon: do the v4l2 will have an analog of fourcc_mod_code macro ?
svarbanov: probably
or we could re-use the DRM one
since compressed modes are likely to be supported by the GPU/display controller as well
bbrezillon: yeah, I'm asking in this spirit drm <-> v4l2 interaction, because the compressed raw formats are vendor specific and most probably the drm macro can be re-used
but it is not clear what drm guys would say for that (having in mind the discussion on the common image library patchset) :)
svarbanov: well, since modifiers is a new thing in V4L2, I don't see any problem re-using the DRM defs
but that's probably a question for hverkuil, pinchartl and mchehab
bbrezillon: svarbanov: one thing at a time :-)  Let's finish the codec (stateful/stateless) API first before going on to the next step (new streaming/fmt ioctls).
New APIs take a *long* time before everyone is happy with them.
svarbanov: regarding tests: they have been developed with vicodec + the latest versions of the stateful codec specs.
Look at contrib/test/test-media for the suite of vicodec tests:
it uses the vicodec encoder to create test streams, then uses those to do the vicodec stateful decoder compliance tests.
It runs that test with various bitstreams: no resolution changes, a resolution change every 2 frames and even a bitstream with a resolution change every frame.
It also does two compliance test runs: one using --stream-from-hdr, and one using --stream-from. The latter option just reads from a bitstream, the --stream-from-hdr reads from a special stream created with v4l2-ctl --stream-to-hdr: there each received buffer is written to a file prefixed with a simple header containing the length of the buffer.
--stream-from-hdr does the same: it reads the header, then reads that many bytes into the buffer before passing it on to the decoder.
hverkuil, but in general (for real hardware) we have to have few ready-to-use bitstreams files, no?
Yes.
If you have encoder HW, then it might be easiest to generate them using vivid (as the source) and v4l2-ctl to write the bitstream.
A moving colorbar from vivid would work very well as a standard test stream. For testing the API we only need short sequences.
Actually, you don't need vivid: v4l2-ctl has a built-in TPG (same as used in vivid).
ok, I'll play with the tests and give you some feedback. If I remember correctly last time when I used the tests they failed early because the test trying to REQBUF(CAPTURE) before even setup the OUTPUT queue, is that fixed now or I have to fix the driver state machine
v4l2-ctl can also generate interlaced formats, that would be useful as well.
I think that should work now. But I am not completely sure.
It's really a two part thing: 1) create sensible test bitstreams, 2) use those to check various drivers and the compliance test.
yes, but IMO it will be easier if we can avoid that, and have these bitstreams already in the v4l-util git
or maybe at least some basic streams
It's the intention to have those bitstreams in v4l-utils.
Yeah, start with a simple bitstream. And a simple script somewhere that can generate them given a HW encoder.
I'll provide you with the v4l2-ctl command line options to setup the TPG.
hverkuil: should we clarify that the DMA-buf FDs returned in v4l2_buffer/plane by DQBUF are just the numerical copy of what was given to QBUF and not some new file descriptors?
the documentation doesn't seem to mention anything about what those are
tfiga: wouldn't hurt.
hverkuil, yes, please give some instructions how to create such bitstreams so that I can try the test on my side
svarbanov: does your encoder support interlaced?
hverkuil, I don't think so :)
(hmm, I need to test that for vicodec)
hverkuil: I'm not sure if it's a good idea to have hardware-encoded bitstreams for such testing
I'd rather use some ffmpeg or so
hardware encoders are not always 100% compliant unfortunately...
hverkuil: as for the FD, let me create a patch
tfiga: yes, ffmpeg is a better idea.
And it's easier to make a script to generate the test sequences that way.
we have some streams in chromium already, if that could be interesting
https://chromium.googlesource.com/chromium/+/refs/heads/trunk/media/test/data/
hmm
that was probably some old ref
https://chromium.googlesource.com/chromium/src/+/HEAD/media/test/data
scroll down for description of those
tfiga, I think those test files are with containers, no? we need elementary streams (for google vp8/vp9 codecs maybe .ivf header)
depends on the file
but still you can just demux them, right?
test-25fps.h264 for example is annexb
tfiga, yep :) but this is extra work which I want to avoid :)
I think you can actually judge by the file extension
svarbanov: perhaps less work than creating new streams?
tfiga, yes I know very well test-25fps* files ;)
but the ones already demuxed should be the most interesting ones
for example: switch_1080p_720p_240frames.h264
tfiga, I agree, but the problem for me is what v4l2-compliance expecting as an input files (it is not clear for me)
tfiga, i.e. how the h264 bitstream is separated to NALs
that's not clear for me either (haven't checked yet) :(
yeah, that was question for hverkuil :)
using option --stream-from will just read from the file and fill each OUTPUT buffer to the max and pass it on to the decoder. I.e. the decoder has to parse it.
if the decoder is expecting that userspace has pre-parsed it (and BTW how do I know that as an application?), then we need something that can parse a bitstream and insert headers indicating the size of each buffer and you use --stream-from-hdr with v4l2-compliance.
svarbanov: what exactly does the venus decoder expect from the input format(s)?
BTW, here is a script that uses vivid to create a raw 1080p60 YUYV file containing 60 frames:
v4l2-ctl -i3 --set-dv-bt-timings index=14
v4l2-ctl -c test_pattern=2 -c horizontal_movement=4 -c osd_text_mode=2
v4l2-ctl -v pixelformat='YUYV'
v4l2-ctl --stream-mmap --stream-count=60 --stream-to=1080p60.yuyv
You can change the pixelformat to something else if that's easier.
The result can be fed to ffmpeg.
hverkuil, it expects in case of h264 at least one NAL unit in the input buffer, if NAL units are more than one they should be aligned on NAL boundary
svarbanov: is that documented anywhere? How would userspace know this?
hverkuil, hm, this is how gstreamer and ffmpeg currently works
But the H264 pixelformat has no such restriction. I don't know how other decoders behave, but AFAIK the exynos mfc doesn't have that limitation.
To compress the raw output file with ffmpeg: ffmpeg -f rawvideo -s 1920x1080 -r 60 -pix_fmt yuyv422 -i 1080p60.yuyv out.mpeg
Updated script:
# selection HDMI input with 720p60
v4l2-ctl -i3 --set-dv-bt-timings index=7
v4l2-ctl -c test_pattern=2 -c horizontal_movement=4 -c osd_text_mode=2
v4l2-ctl -v pixelformat='YUYV'
v4l2-ctl --stream-mmap --stream-count=60 --stream-to=720p60.yuyv
ffmpeg -f rawvideo -s 1280x720 -r 60 -pix_fmt yuyv422 -i 720p60.yuyv 720p60.mpeg
# 1080p60
v4l2-ctl --set-dv-bt-timings index=14
v4l2-ctl --stream-mmap --stream-count=60 --stream-to=1080p60.yuyv
ffmpeg -f rawvideo -s 1920x1080 -r 60 -pix_fmt yuyv422 -i 1080p60.yuyv 1080p60.mpeg
cat 720p60.mpeg 1080p60.mpeg >mix.mpeg
hverkuil, looking in this git, I'm not sure about your statement about such limitation in mfc driver  http://git.infradead.org/users/kmpark/public-apps/tree/HEAD:/v4l2-mfc-example
see parser.c
svarbanov: that parser could be used to make a converter that converts a bitstream into a hdr format that v4l2-compliance can use.
The header format is just the FILE_HDR_ID 32 bit value (network order) followed by the length (network order) followed by the data that should go into the buffer.
hverkuil, yes, I thought for something like that too
svarbanov: hverkuil: some older MFC revisions required the bitstream to be cut into pieces
pH5: I see that patch 8/10 doesn't have Rob Herring's Ack yet. I'll merge the series up to patch 7.
tfiga, but for newer versions you don't need this to split the the stream, right?
pH5: ping
pH5: are the Documentation/devicetree/bindings/media/rockchip-vpu.txt bindings still valid after the rename to hantro?
I think it's OK, but I would like to have that confirmed before I post the PR.
hverkuil: yes, the imx8m binding still needs rob's blessing, and yes, the rockchip vpu binding is unchanged.
pH5: thanks for the confirmation! I've posted a PR for patches 1-7.
pH5: BTW, I'll try to work on getting my i.MX8MM in shape for testing this on Friday.
hverkuil, paulk-leonov, ndufresne: the set of ctrls for all stateless XXX codecs (replace XXX by the bitstream format: H264, HEVC, VP8, ...) should be the same right?
hverkuil: thanks!
bbrezillon, ezequielg: ^ sorry for the delay, I was out of office last week.
I'm trying to see if having an generic array of ctrl def per codec type makes sense
pH5: np
and thanks for sending v5
pH5: hmm, there is no support yet for that board in u-boot, it seems.
bbrezillon: I don't think you can count on that.
hverkuil: but some of them are mandatory, right?
otherwise it would make userspace life a lot more complicated
Yes, but that differs per codec.
It would actually be nice if the documentation would specify which controls are mandatory. It doesn't do that if I remember correctly.
pH5: hverkuil: great, thanks.
hverkuil: oh, yes, it's a per codec thing of course
hverkuil: I see two patch series, https://patchwork.ozlabs.org/cover/1031790/ and https://patchwork.ozlabs.org/cover/1093140/, but from the comments they don't seem to be ready yet.
what I meant was that all drivers implementing H264 decoding should expose all mandatory controls
for H264 decoding
Yes. Provided we'd know what the mandatory controls are, and I don't think that's explicitly specified.
hverkuil: looks like cedrus has it marked its implementation
*in its
bbrezillon: are you talking about stateless codecs or stateful or both?
I have stateless codecs in mind
but I guess this might apply to stateful codecs too
Ah: stateless codecs explicitly define which controls must be present in the request.
I thought you were talking about 'regular' codec controls.
ok, so it definitely makes sense to have those mandotory (and maybe optional) controls defined in a central place
hverkuil, paulk-leonov, ezequielg: it's still WIP, but this is where I'm heading to with this m2m-codec layer https://github.com/bbrezillon/linux/blob/media/m2m-codec/include/media/v4l2-mem2mem-codec.h
bbrezillon: to answer your question from earlier: the set of controls depends on the codec
if not only because of different terminology
but also added features
and I plan to have per-codec specific helpers https://github.com/bbrezillon/linux/blob/media/m2m-codec/include/media/v4l2-mem2mem-mpeg2-codec.h
bbrezillon: fwiw, i truly dislike the v4l2_m2m_codec_run struct.
maybe make it "v4l2-mem2mem-codec-mpeg2" so that the left-to-right order matches the logical imbrication order?
ezequielg: hehe
i'd rather see the parameters explicitly passed around.
ezequielg: Kwiboo liked it
and passing params around means we can't easily add new ones
other than that, I don't see any problem letting drivers call v4l2_m2m_codec_get_ctrl(id)
instead of having this struct
it's just that cedrus was doing it like this, and Kwiboo asked me a few weeks back if we could convert the rockchip driver to this approach
ezequielg: I think it's easier for everyone to have helpers to retreive the data structures from controls
bbrezillon: hehe, i explicitly avoided the _run thing in the rockchip one.
guess it's all a matter of taste.
I don't really like the _run thing in cedrus much
it's really hard to see how functions depend on parameters if you have a central big struct with a bunch of things.
specially since parameters are documentation for me.
to be honest, if this is the only thing that bothers you, I'll be happy to change it :)
yeah, let's not bikeshed much at this point.
ezequielg: I disagree with you though, I think it's beneficial to have the helpers retrieve the structs data from the controls for the driver
either way we need a helper to pass each per-slice controls anyway
IMO we should have some matching between ops and controls
ops?
such as a per-slice driver callback that is given the per-slice controls as structures
bbrezillon: yeah, what I had in mind with helpers is ops for each step of configuring the pipeline
which closely matches the controls
a bit like we do in cedrus
yes, still haven't take a decision about that one
right now it's left to the driver
indeed
but you have a way to store ops in a private field
I'd like that to be part of the common helpers
in the coded_fmt_desc
currently in cedrus we have a "setup" step
I think it should be split to represent parts of the bitstream that can be configured somewhat independently
at least with a separation between per-slice elements and per-frame elements
but anyway, I can pick that up later if needed :)
no, that's good to discuss it now
would those ops be per-codec?
bbrezillon: yes I think they have to be
the concept of slice/frames you're talking about do not apply to all formats (VP8 only does frame decoding)
indeed
ok, then I guess it's a per-coded fmt thing
paulk-leonov: everything is run through the ->run() hook anyway, right?
bbrezillon: yeah in the end it's m2m's device_run
paulk-leonov: really looks like a driver/HW specific thing
ideally we could have a generic helper there with driver-specific callbacks
(at least if I refer to the cedrus driver)
sure, that's where hardware configure needs to happen
configuration*
the point is that it's pretty much the same configuration always
only done different depending on the hardware
no, but I mean, even the split you've done seems to be driven by cedrus HW
the one I have in mind or the one we currently have in cedrus?
the current one
yeah it sure
is
do you maybe have something in mind for say H264 and MPEG2
?
I thought about adding per-coded fmt ops, but for a different reason
and I had something codec-independent in mind
mhh only some vague idea
like, we need to allow drivers to allocate per-coded-fmt context on start_stream and free it on stop_stream
looks like a good point to hook that yes
and we also probably want to have a ->run() that's called by the driver
I like the idea of a final trigger op
like multiple ops for codec configuration and a final trigger op
what would be the multiple ops
?
(don't need a super precise example, just something that gives me an idea of what it would look like)
e.g. for mpeg-2 there would one for slice params, one for picture params and one for quantization
hm, not sure that's a good idea
you sometimes need all info to take a decision
(that's true at least for H264 and the hantro block)
having on hook per control means drivers would just store the controls in some priv struct (the _run struct you don't) like
and flush things at trigger time
bbrezillon: right right, that's not necessarily to say that you only get these controls, it's more to have understanable steps that are likely to be grouped in the register layouty
so more like ops per groups of registers
ideally you could just retrieve cached controls
sounds like something that's HW-specific
so you don't need explicit storage of them
(I mean, the concept of "group of regs")
bbrezillon: yeah it's half-way between codec semantics and hardware implementation
which is were drivers live :)
where*
then, I think it's better to keep those driver ops in the driver itself
and not try to make something generic out of it
mhh I think I disagree but we probably need a more formal basis to really have a clear idea
as I said, it's just pretty vague, but I have a feeling that we'll end up with more or less the same steps
and the point is really to help with driver readability
and making it easier to implement a new one
so it certainly shouldn't be a constraint
like, it shouldn't forbid doing certain things in the implementation
just help lay things out in a common understandable way
so in the end I think this would be more about cosmetics
yes, that's the whole idea behind those helpers, it's just that I don't see the need for those fined-grained per-codec ops
it might be a bit early since we only have 2 drivers sofar, but it could become a problem later
anyway if you don't feel the need we can come back to it later
agreed, nothing is set in stone anyway
if I can come up with something that avoids 50% of the code dup we have in the hantro/rockchip and cedrus drivers that's already a step forward
bbrezillon: related to the run struct, you are correct in that I preferred passing a run object instead of having to pass 2-3 ctrl because I needed to access different controls in a h264 function 2-3 levels down, changing function params everywhere is annoying, also your codec specific run struct looks much better then the cedrus all-for-one version
paulk-leonov: so, back to my initial question, is it okay to assume that all drivers will have the same set of optional/mandatory ctrls on a per-codec basis
?
bbrezillon: (sorry for the delay) I would assume so yes, except in cases where specific controls are tied to optional features that the decoder may not support (e.g. scaling lists)
paulk-leonov: but if it's flagged as optional, it's also not a problem to expose it
is it?
hm, just realized while writing it that it's actually a problem
okay, I'll see what I can do
hverkuil, fyi https://paste.ubuntu.com/p/SdtQbQJBBj/
hmm "Video Capture Multiplanar cap set, but no Video Capture Multiplanar formats defined", that's not a bug, a driver can use multiplaner buffer type with single planar formats
Unless it means that there is no format on the MPLANE variant of the enum ?
svarbanov, note, in gstreamer I have a hack, I just pick the format from both type
ndufresne, I test it on current media/master so it could be related to "media: v4l2: Get rid of ->vidioc_enum_fmt_vid_{cap, out}_mplane"
but not sure though
svarbanov: do you have this patch in your tree? media: v4l2: Fix the _MPLANE format check in v4l_enum_fmt()
bbrezillon: that's one thing we have left to formalize a bit more, what's optional and what's not
svarbanov: was merged in master two days ago.
svarbanov, oh, right
(sorry for my noise, hverkuil the error message could be improved)
hverkuil, no I haven't, now rebasing
mchehab: that's quick! Thanks for merging the hantro rename series, that make life easier.
:-)
yeah, it makes sense to apply those renaming patches quickly
hverkuil, something is not ready :) https://paste.ubuntu.com/p/nMy7SHrMYG/
svarbanov: the VIDIOC_(TRY_)DECODER_CMD: FAIL test is probably easy to fix by using the v4l2_m2m_ioctl_try_en/decoder_cmd helpers.