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