<!-- 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>
Renault: <u>hverkuil1</u>: SDI/GS1662 patch is good enough? Because my company has some troubles in imminent future and it should be difficult to test another patchset
hverkuil1: <u>Renault</u>: I'll take a look today.
Renault: thank you (sorry for that, but I prefer to be sure to fix the driver if necessary while hardware is available)
hverkuil1: no problem, thanks for letting me know.
<br> <u>Renault</u>: looks good, just one comment about the doc. See the reply I posted.
Renault: ok, thank you
<br> a merge in 4.8 is possible? Maybe too late
hverkuil1: No, it'll be 4.9.
Renault: ok :)
pinchartl: <u>hverkuil1</u>: ping
hverkuil1: <u>pinchartl</u>: pong
pinchartl: you mentioned in your review of the FDP1 driver that the deinterlacing mode control should probably be turned into a standard control
hverkuil1: yes
pinchartl: while I agree that the feature should be fairly common, the deinterlacing modes themselves are mode device-dependent
<br> I haven't played with many deinterlacers, so I'm not sure what a common base would be
hverkuil1: You can look at what e.g. vlc supports in the way of deinterlacers.
pinchartl: but that's software, not hardware
hverkuil1: The underlying principles are the same, there are only so many ways you can do deinterlacing.
<br> Do you have some documentation for the FDP deinterlacing modes?
pinchartl: nothing public, but I can briefly explain them
hverkuil1: (BTW: the best solution is to NOT USE BLOODY INTERLACED VIDEO! It's evil!) :-)
pinchartl: no need to convince me :-)
<br> let me open the documentation that I can't send you :-)
hverkuil1: Also see https://en.wikipedia.org/wiki/Deinterlacing
pinchartl: there are 6 modes
hverkuil1: does the deinterlacer HW use memory to store older fields? (just curious)
pinchartl: the progressive mode is special in the sense that it does nothing. the hardware just passes the input frames to the output
<br> the hardware uses system memory
<br> depending on the mode it can access multiple fields to reconstruct one image, yes
<br> it can also store intermediate data
hverkuil1: OK, so that makes it a fairly fancy deinterlacer.
pinchartl: the "Fixed 2D" mode uses the current field only and performs line doubling
<br> (possibly with interpolation, I'm not sure about that)
hverkuil1: Very standard technique.
pinchartl: it's a classic bob deinterlacer
hverkuil1: Never use it :-)
pinchartl: the "Previous field" and "Next field" use two fields to construct one image
<br> as the names imply, an image is constructed from the current field and either the previous field or the next field
<br> with input fields transmitted as A B C D E...
<br> it will generate frames that contain AB BC CD DE ...
hverkuil1: OK, so the output is double the amount of data.
<br> That's what the adv7180 deinterlacer does as well (discussed yesterday).
<br> <u>Note</u>: to support that we would also need to add new V4L2_FIELD_ value(s), since this mode cannot be represented today.
pinchartl: the two other modes are less clear
<br> "Fixed 3D" and "Adaptive 2D/3D"
<br> they both use the current, previous and next fields
<br> the adaptive mode also stores intermediate data for an undocumented purpose
<br> the adaptive method combines 2D and 3D deinterlacing, with motion detection and diagonal interpolation
<br> the intermediate data buffer seems to contain a still mask
<br> so it's related to motion detection
<br> that's all I know
<br> I'm not sure what the Fixed 3D mode is exactly
<br> I also don't know if the "Fixed 2D" method is a simple bob deinterlacer, or if it performs interpolation
hverkuil1: I'm considering whether we should do the same as V4L2_CID_TEST_PATTERN, i.e. a standard CID, but a per-driver custom menu.
<br> Or do as V4L2_CID_SCENE_MODE where we just keep adding modes.
pinchartl: for test patterns I don't think it would scale
<br> for deinterlacers, the issue is that I don't know what the hardware does exactly
<br> so it's difficult to standardize modes :-)
hverkuil1: That the main problem with HW deinterlacers: they generally never tell you the details.
<br> BTW, what do you mean with "I don't think it would scale" to use the V4L2_CID_TEST_PATTERN approach?
<br> not scale in what respect?
pinchartl: I don't think it would scale to standardize the menu items and keep adding new ones
<br> there's too many differences between devices when it comes to test pattern generation
hverkuil1: Exactly, V4L2_CID_TEST_PATTERN uses custom menus, a different one for each driver.
<br> It uses v4l2_ctrl_new_std_menu_items().
pinchartl: sorry, I had misread you
<br> doing the same would make sense I believe
hverkuil1: It is a bit of a shame that some modes (like line doubling) are very standard, but with v4l2_ctrl_new_std_menu_items you can't really standardize them.
<br> Did you notice that the LWN statistics for the 4.7 kernel had NO media developers among the 'most active' developers? I think that's the first time in many years.
<br> I think we'll compensate for that in 4.8 though :-)
pinchartl: I haven't checked the stats, no
<br> I've been too busy working on non-mailine code :-(
<br> <u>hverkuil1</u>: when do you plan to get V4L2_PIX_FMT_FLAG_REQUEST_CSC merged ?
hverkuil1: I have no plans for the near future. But feel free to pick it up if you need it.
pinchartl: I was planning to allow applications to set the colorspace on the capture queue without that flag :-)
hverkuil1: that wouldn't work (or be accepted). Is this for your non-mainline work?
pinchartl: no, it's for the FDP1 driver
hverkuil1: Just take that patch. I tested it at the time, I may even still have some v4l2-ctl patches that add support for it.
<br> There is some code here, but it is for qv4l2: https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=colorspace3
<br> Old code though, not sure if it is still correct.
pinchartl: I need to investigate that
<br> as replied by e-mail, the hardware supports configurable quantization when writing to memory
<br> but doesn't when reading from memory
<br> so I'm wondering if the quantization at the output of the deinterlacer should really be made configurable by userspace
<br> or if it should always be set to the same quantization method as configured on the read side
hverkuil1: Just to be sure I understand the hardware: userspace feeds it top and bottom fields (one per buffer), and you get deinterlaced frames back?
pinchartl: yes
<br> well
<br> either one per buffer
<br> or two per buffer
<br> let me check
<br> the hardware processes one field per buffer
<br> but the stride is configurable
<br> so buffers can store two fields in interlaced or sequential format
<br> in which case the driver will feed them twice to the hardware
<br> one for each field
hverkuil1: Ah, OK. Good to know.
pinchartl: the ALTERNATE field types are not supported by the driver at the moment
<br> they could from a hardware point of view
<br> but gstreamer doesn't seem to support that yet
<br> (that's what I've been told by Kieran)
hverkuil1: With FIELD_ALTERNATE you mean the one-field-per-buffer mode? (alternating top and bottom fields)
pinchartl: yes
<br> it looks like the driver has partial support for V4L2_FIELD_ALTERNATE
<br> but S_FMT doesn't accept it
<br> <u>hverkuil1</u>: using the height as a heuristic... that feels sooooo wrong :-)
hverkuil1: Effective, though :-)
<br> I think it is worthwhile supporting FIELD_INTERLACED with that heuristic.
<br> If you capture from e.g. a composite input, then what you receive will use FIELD_INTERLACED, so it would be nice if you can just feed it into this deinterlacer, keeping the field setting the same.
pinchartl: why do we have FIELD_INTERLACED in the first place ?
<br> shouldn't drivers always report _TB or _BT ?
<br> <u>hverkuil1</u>: v4l2-compliance complains that the colorspace on the capture video node differs from the colorspace on the output video node
<br> when the device converts data from YUV to RGB, what's the expected colorspace ?
hverkuil1: _TB and _BT are quite new. They were added to handle cases where the capture device incorrectly gave the wrong Top/Bottom ordering. The original FIELD_INTERLACED is much older.
pinchartl: ok
<br> it's a mess :-)
hverkuil1: The colorspace remains the same. It is only ycbcr_enc and quantization that can change.
<br> The colorspace refers to how the red, green and blue colors should be interpreted (i.e. which color red is meant exactly?).
<br> The conversion to Y'CbCr is just a different encoding of RGB and it doesn't change the colorspace.
<br> In 99% of the cases FIELD_INTERLACED is all you need, so it does make sense.
pinchartl: right
<br> but ycbcr_enc and quantization will not match between the output and capture nodes when converting from yuv to rgb, right ?
hverkuil1: They will if they use the DEFAULT value (0).
pinchartl: but not if they use non-default values
hverkuil1: let me check v4l2-compliance...
pinchartl: v4l2-compliance requires a match
<br> I don't think that's correct
hverkuil1: You are right, this code doesn't work if there is a YUV to RGB or vice versa conversion happening.
pinchartl: it would have been too easy :-)
<br> also
<br> I think the handling of DEFAULT values is underspecified
<br> for instance
<br> if I S_FMT()
<br> on an output video node
<br> with a colorspace
<br> and DEFAULT for the encoding and quantization
<br> and then G_FMT()
<br> the driver could return DEFAULT
<br> or resolve the DEFAULT to the default values for the colorspace
<br> both would be valid, wouldn't they ?
hverkuil1: I'm thinking that the ycbcr_enc and quantization checks at the end of testM2MFormats should be skipped if the pixelformat changed value. Since that can indicate an encoding change. That seems reasonable.
pinchartl: I think I agree
<br> (it's the only v4l2-compliance test that fails with the driver ;-))
hverkuil1: Yes, drivers can return either DEFAULT or the actual value.
<br> The compliance test currently assumes that that doesn't happen.
<br> Part of the reason is quite annoying: there still is no way to know if the pixelformat is RGB or YCbCr, and that information is needed to figure out what DEFAULT actually maps to.
<br> I proposed adding a flag for that in the past, but it never got in.
<br> For ycbcr_enc you can do it, since that only depends on the colorspace, but for the default quantization you need to know if it is rgb or ycbcr.
pinchartl: brb (phone), sorry
<br> back
<br> <u>hverkuil1</u>: regarding the ambiguity
<br> should drivers keep the DEFAULT value, replace it with the actual value, or be allowed to pick the behaviour they want ?
hverkuil1: Both are allowed, but in practice drivers keep the DEFAULT value. It would be my advice as well: don't unnecessarily change DEFAULT to the actual value.
<br> I have thought of explicitly documenting this, but I feel this should remain a recommendation and not something compulsory.
pinchartl: internally drivers need to compute the actual value
<br> so they would need to keep track of the original request, and of the real value
<br> not a really big deal, but a bit annoying
hverkuil1: That's what I did as well.
pinchartl: keeping both ?
hverkuil1: yes
pinchartl: I wonder whether the V4L2 core shouldn't compute the real value before passing it to drivers
hverkuil1: well, usually there is only one place where I actually need the real value, so it can be just a local variable.
<br> I've thought about it, but there are so few drivers that actually set this (as opposed to just reporting it), that I didn't bother.
<br> That might change in the future.
pinchartl: but in any case, userspace should be able to handle both, right ?
hverkuil1: Yes.
<br> qv4l2 does.
pinchartl: do you plan to fix v4l2-compliance ?
hverkuil1: If my proposed fix works, then just commit it. It's trivial.
pinchartl: which fix ?
hverkuil1: skipping the ycbcr_enc/quantization checks if the pixelformat is no longer the same.
pinchartl: and handling the default -> non default change ?
***: prabhakarlad has left
pinchartl: <u>hverkuil1</u>: when outputting RGB what ycbcr_enc should the driver set ?
hverkuil1: <u>pinchartl</u>: 0 (DEFAULT).
<br> It really is ignored, but it is really impolite not to set it to 0.
pinchartl: ok :-)
<br> <u>hverkuil1</u>: I'll bother you again if I may
<br> so my hardware supports three combinations of encoding and quantization
<br> V4L2_YCBCR_ENC_601 + V4L2_QUANTIZATION_FULL_RANGE, V4L2_YCBCR_ENC_601 + V4L2_QUANTIZATION_LIM_RANGE, V4L2_YCBCR_ENC_709 + V4L2_QUANTIZATION_LIM_RANGE
<br> how should I limit the accepted colorspaces ?
<br> for instance, should the driver accept V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_601 ?
<br> or does V4L2_YCBCR_ENC_601 make no sense with V4L2_COLORSPACE_BT2020, in which case V4L2_COLORSPACE_BT2020 should be rejected ?
hverkuil1: You only have to look at the ycbcr_enc and quantization fields for this. The only thing that the hardware can convert between are those two. The colorspace and xfer_func remain fixed.
pinchartl: I agree with you
<br> but does it make sense to have V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_601 for instance ?
<br> or is that a combination that can't exist and should thus be rejected ?
<br> the hardware doesn't care about the colorspace or the transfer function
<br> but should the driver reject combinations that don't make sense ?
hverkuil1: It's possible theoretically. It will normally never happen (BT,2020 defines its own YCbCr encoding), but the chosen YCbCr encoding is from the point of view of V4L2 independent of the colorspace.
<br> The four properties (colorspace, xfer_func, ycbcr_enc and quantization) are all independent and can theoretically occur in any combination.
pinchartl: so you would accept combinations that don't make sense as long as the encoding and quantization can be supported by the hardware ?
<br> theoretically, yes
hverkuil1: Yes.
pinchartl: so if userspace requests
<br> V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_DEFAULT
hverkuil1: Vivid allows any combination, for example.
pinchartl: that should normally lead to V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_BT2020
hverkuil1: right.
pinchartl: but V4L2_YCBCR_ENC_BT2020 isn't supported
<br> by the hardware
<br> so should the driver then consider V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_DEFAULT to be equal to V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_601 ?
hverkuil1: But if the deinterlaced just deinterlaces and never does any CSC conversion (i.e. the matrix is the identity matrix), then it will just output V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_BT2020. No need to prohibit that.
pinchartl: or rather, when asked V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_DEFAULT in S_FMT, return V4L2_COLORSPACE_BT2020 + V4L2_YCBCR_ENC_601 ?
<br> yes, I was just thinking about that
<br> the encoding is only limited to 601 and 709 when outputting RGB
hverkuil1: Trying to convert from V4L2_YCBCR_ENC_BT2020 to ENC_601 or 709, that is illegal.
pinchartl: which is quite interesting, as at S_FMT() time on the output queue, I don't know whether YUV or RGB will be selected on the capture queue
<br> S_FMT() is called on the output queue first, and then on the capture queue
<br> I could disallow RGB capture if the encoding isn't 601 or 709
hverkuil1: Right, that's what I had in mind as well.
pinchartl: I'll try that
<br> I'll accept anything on the output queue
<br> and on the capture queue I'll only allow RGB if the encoding + quantization match
<br> that should be doable
hverkuil1: Sounds good.
<br> Colorspaces are fun! :-)
pinchartl: I'll have to rewrite the format handling completely, it's wrong
hverkuil1: If you want me to review code with an eye on colorspace handling, just let me know.
pinchartl: I'll ask for your review when I'll post the next version
<br> thanks