Created attachment 300358 [details] [review]
v4l2: videodec: choose format from caps

Instead of just copying the format and frame size, query the peer caps filtered with the current v4l2 capture queue caps and set that format on the capture queue.

I'm unsure about two things. First, the v4l2capture->probed_caps (cached since device open) are useless because the currently available caps are already limited by the encoded stream format. I drop the cache and then call gst_v4l2_object_get_caps - this should be done better, how?
Second, gst_caps_fixate choses the smallest possible values for width and height, when usually the frame size on the input side should be preferred.

Did you implemented late probe as discussed in Dusseldorf (changing reported format base on the input) ? The cache is still relevant, but after the OUTPUT format has been fixed we should update the cache once. This way we don't do slow probing on every get caps query. Note I didn't review the patches yet, I'm just commenting on the comment.

About fixate, I didn't review it, but I kind of understood that CODA does not really scale. So we should not negotiate and fixate width and height in this element. At least not until we got a clear idea how that is supposed to work.

Review of attachment 300358 [details] [review]:

::: sys/v4l2/gstv4l2videodec.c
@@ +521,3 @@
+
+    /* TODO: prefer input values instead of smallest width/height/framerate */
+    caps = gst_caps_fixate(caps);

That's kind of wrong, we should somehow acquire what the driver would do without action, and fixate toward this. Width/Height for now should we fixed to what the driver says by default.

Currently I have not implemented ENUM_FMT(CAP) returning only a limited selection after STREAMON(OUT), mostly because only JPEG decoding (not mainline ready yet) needs it. S_FMT/TRY_FMT(CAP) limit the size to a single setting.

Would your suggestin be to clear the cache not before calling gst_v4l2_object_get_caps(v4l2capture), but instead after gst_buffer_pool_set_active(v4l2output->pool)? That's where the JPEG decoder will know whether it was fed 4:2:2 or 4:2:0 subsampled JPEG images, just a few lines above.

Should I add back the call to gst_v4l2_object_acquire_format(v4l2capture) and then use the returned width and height to limit the filter like the framerate?

(In reply to Philipp Zabel from comment #4)
> Currently I have not implemented ENUM_FMT(CAP) returning only a limited
> selection after STREAMON(OUT), mostly because only JPEG decoding (not
> mainline ready yet) needs it. S_FMT/TRY_FMT(CAP) limit the size to a single
> setting.

I'm not sure what this change will give you then. Can you explain the initial problem ?

> 
> Would your suggestin be to clear the cache not before calling
> gst_v4l2_object_get_caps(v4l2capture), but instead after
> gst_buffer_pool_set_active(v4l2output->pool)? That's where the JPEG decoder
> will know whether it was fed 4:2:2 or 4:2:0 subsampled JPEG images, just a
> few lines above.

I think you should cache two things. The probed caps for this OUTPUT format, and keep the probed_caps for this device. And simply use the right one when needed. You could have a new method probe_caps() that ignores the cache. It's not clear to me if the existing driver will return return something different when probed, in which case the resulting intersection with downstream caps may and blind fixate may result in a failing set_format(). We need to check that, not only for CODA.

> 
> Should I add back the call to gst_v4l2_object_acquire_format(v4l2capture)
> and then use the returned width and height to limit the filter like the
> framerate?

This element is currently the only user of this method. You could pass the queried downstream caps and make it negotiate somehow. This would help to avoid unwanted side effects.

The coda can decode h.264 into either I420 or NV12, but currently only I420 can be used because v4l2videodec never calls S_FMT(CAP) with a format negotiated with the src pad peer.

I need to do (roughly) the following:
1. S_FMT(OUT)
2. REQBUFS(OUT)
3. STREAMON(OUT)
4. QBUF(OUT)
5. G_FMT(CAP)
6. ENUM_FMT(CAP) <- chose one of these formats depending on src pad peer
7. S_FMT(CAP)
8. REQBUFS(CAP)
9. STREAMON(CAP)
The v4l2videodec element currently doesn't to 6. and 7., which this patch intends to add. I can't use the cached probed_caps in place of 6. because the driver may have disabled some formats depending on the encoded data.

Created attachment 300464 [details] [review]
v4l2: add gst_v4l2_object_probe_caps

Add a variant of gst_v4l2_object_get_caps that bypasses the probed_caps cache.

Created attachment 300465 [details] [review]
v4l2: videodec: choose format from caps

Keeps the call to acquire_format and uses the resulting info.width/height and
input_state->info.fps_n/d to fixate the caps, and sets them.

There is no necessary delay between steps 4 and 5 right?

The driver should send a V4L2_EVENT_SOURCE_CHANGE event, the v4l2videodec element should wait for that. But right now only s5p_mfc implements this.

(In reply to Philipp Zabel from comment #10)
> The driver should send a V4L2_EVENT_SOURCE_CHANGE event, the v4l2videodec
> element should wait for that. But right now only s5p_mfc implements this.

MFC driver also blocks in G_FMT until the header is fully parsed.

What I don't understand is that in comment 4 you said you didn't implement 6. Considering drivers usually don't implement 6, how is this suppose to not break backward compatibility ?

Review of attachment 300465 [details] [review]:

This is still very incomplete. What about the other structures in the downstream query ? What if set_format() fails ? You aren't forcing width/height to not change.

Review of attachment 300464 [details] [review]:

This looks like a good idea, I'll merge when we have code using it.

Created attachment 300587 [details] [review]
v4l2: videodec: choose format from caps

Enforce input width/height/framerate via gst_caps_set_value, goto not_negotiated if gst_v4l2_object_set_format fails.

Review of attachment 300587 [details] [review]:

::: sys/v4l2/gstv4l2videodec.c
@@ +548,3 @@
+    if (!gst_v4l2_object_set_format (self->v4l2capture, caps)) {
+        gst_caps_unref (caps);
+        goto not_negotiated;

Somehow this change have to be wrong, if acquire_format() didn't fail, we should never fail here. I suggest to start brainstorming on a design (which I haven't had time to think of yet).

Obviously this should never fail. If it does, it is a driver bug which we can't recover from. I added the check because of comment 12: "What if set_format() fails ?"

I think in this case, an older kernel will fail because you will pick from a list of formats that have not been reduced. We can't change the spec and then say all driver are broken. So it should never fail and be backward compatible. So no, it's not strictly driver fault.

I've been searching a bit, and it seems that proper way to negotiate here is to create a caps from the acquired format (will be a single structure fix caps). Remove the format field from the caps and use that as a filter when doing the caps query. Now, what you'll get is a list of structure, with format field fixated or note, but within the set you have passed. You can then take the first structure and fixate (use the acquired format passed to fixate_nearest_int(), this will ensure we keep driver preferred. 

From these caps, you will set_format(), but on failure you should fallback to what the driver initially proposed (not fail the pipeline like you do).

Created attachment 310501 [details] [review]
v4l2: videodec: choose format from caps

Now this creates a single structure caps from the info returned by acquire_format, removes the format field from it, uses that as a filter to probe_caps and queries peer caps with the result as a filter.
If the returned caps is empty, a negotiation failure is reported, otherwise a SET_FMT is tried and if that succeeds, the original GstVideoInfo is replaced from the negotioated caps.

Created attachment 316259 [details] [review]
v4l2: videodec: choose format from caps

Additionally to the format, also drop the colorimetry from the acquired format.

Review of attachment 316259 [details] [review]:

Seems correct from reading it. I'll give it a quick try.

Review of attachment 300464 [details] [review]:

.

Review of attachment 316259 [details] [review]:

.

Attachment 300464 [details] pushed as cf29e6c - v4l2: add gst_v4l2_object_probe_caps
Attachment 316259 [details] pushed as c75a69d - v4l2: videodec: choose format from caps

Thanks for this effort, it's great to finally have support for this.