can VCHI frame timestamp values be made opaque? (not usec)

[alien999999999]

While upstreaming bcm2835-codec, Nicolas Dufresne noted that the timestamp in v4l2 is not necessarily an actual time value (in nsec). In fact, GStreamer actually uses a frame counter for this field. (see https://lore.kernel.org/linux-media/[email protected]/T/#m4521f17f9e75074ff3993df4a23c146c26f6e2a6 )

I don't know how the firmware handle the mmal(VCHI?) timestamp values, does it actually calculate something with these timestamps?

If it doesn't, it might be easy enough to get a fix in the firmware to not "interprete" these timestamp values, which would make the mmal code a lot simpler and could avoid potential issues (eg: GStreamer)

The code does state in comment that the mmal timestamps are usec, so if it does need usec timestamps and it needs this for a specific calculation, maybe we can add an extra field?

[6by9]

The firmware does process timestamps under a number of situations. The ones that immediately come to mind are:

• Video decode has some code to fix up timestamps when we're fed incorrect PTS values on B-frames. I believe that is disabled though when using the V4L2 interface.

• Video encode can run without a fixed framerate being specified, and rate control then looks at the timestamps to assign bits to frames.

• Deinterlace will take a single interlaced frame (ie 2 fields) in and produce 2 output frames. Those output frames need appropriate timestamps, so I believe it will interpolate between the timestamps provided on 2 input buffers.

[popcornmix]

I remember for deinterlace at least, we added a mode for this. The firmware commit for this (back in Sep 2021)

    di_adv/di_fast: Support mode without timestamp interpolation
    
    v4l2m2m driver reports V4L2_BUF_FLAG_TIMESTAMP_COPY so we shouldn't be meddling with them.
    It's also easier to distinguish NO_PTS values when they are returned.
    
    Negative default_frame_interval would not currently be used so this shouldn't break anything

and the kernel side is here.

[alien999999999]

I guess the negative is about the s64 vs u64 ? but, I assume this means that the firmware does not actually do anything with the timestamps (for all the roles?) and so I could just pass this along without the 1000 div in bcm2835-codec ? or is that only for the deinterlacer?

[6by9]

popcornmix knows more about the deinterlacing use case as it gets used in Kodi / Libreelec.
The negative value bit is https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/staging/vc04_services/bcm2835-codec/bcm2835-v4l2-codec.c#L2672-L2686 when -1 is passed as a parameter to the firmware, not that the timestamps themselves are negative.

Based on a quick read through of the current configurations, I think you're safe removing the timestamp conversion.

[popcornmix]

Deinterlace is a little more complex, as it typically doubles the framerate, so the additional frames get an interpolated timestamp (e.g. timestamp of base frame + default_frame_interval / 2).

As v4l2 wanted V4L2_BUF_FLAG_TIMESTAMP_COPY we added a flag (setting default_frame_interval to -1) to say just copy the same passed in timestamp to both output frames.

I think with the settings the downstream v4l2 driver uses, we are just copying timestamps, so you don't need to scale them, and could in fact use opaque values (like frame counter).

[alien999999999]

ok, good news, I'll definately give it a try; also, wont both of these frames have an identicaly timestamp then? would that be problematic? maybe if someone selects deinterlace on purpose, i guess they would expect double timestamp frames?

[popcornmix]

if someone selects deinterlace on purpose, i guess they would expect double timestamp frames?

It's not a problem for the v4l2 kernel driver. It's up to userspace to handle this.
And userspace should expect double output frames when using deinterlace, and they should expect the same timestamp due to V4L2_BUF_FLAG_TIMESTAMP_COPY.

[alien999999999]

So, i found this old commit raspberrypi/linux@17fa469 where it introduces the nsec -> usec conversion due to encoding rate control (not sure decoder has a similar issue?)

But, how would encoding rate control work if the timestamps were frame counters instead of actual timings?

I'm basically reverting this commit, but then in the newer firmwares, i donno if this breaks encoder rate control...

[6by9]

That conversion was added before the G_PARM/S_PARM ioctls were added in raspberrypi/linux@e6c4cec.

The encoder rate control algorithm will work with either timestamps or a predefined framerate. Based on a quick check it looks like the framerate takes precedence over timestamps, so that should work fine as the V4L2 driver will always set a framerate (defaults to 30fps).
I suspect I tested encode without setting framerate first and therefore noted the units mismatch on the timestamps, but didn't revert it again after implementing framerate controls.

[alien999999999]

that's awesome news, i had already reverted it on my version, and it seemed to work, but i didn't test any rate control yet.

PS: a bit off-topic, but i have off-by-one on fluster decoder testing (vs the software decoder), is there a config setting i can use to fix this?

[6by9]

PS: a bit off-topic, but i have off-by-one on fluster decoder testing (vs the software decoder), is there a config setting i can use to fix this?

Off-by-one on the individual pixels so CRCs fail?
Yes it's the use of the ISP to convert the output frames into the chosen format which has the odd rounding error. I did create a firmware patch that added an option to revert to using the VPU to unpack.
Any firmware after Sept 2023 should allow adding vd_isp_disable=1 to config.txt to use that.

[alien999999999]

thx!

btw: should we close this? or are you planning on reverting this on the raspberry pi kernel too? I'll have this reverted when i do my next RFC on bcm2835-codec on linux-media

[6by9]

I'm not intending on updating anything here, but will adapt to whatever gets merged upstream as and when it happens.

FWIW I'd suggest renaming it to bcm2835-m2m when you next upstream it to make Nicolas happier. Yes only 2 use cases are codecs, but they all follow the memory-to-memory interface.
Deinterlace and simple ISP have no extra requirements (unlike the decoder and encoder), so IMHO there's little point in pulling them out into separate files.

[alien999999999]

FWIW I'd suggest renaming it to bcm2835-m2m when you next upstream it to make Nicolas happier. Yes only 2 use cases are codecs, but they all follow the memory-to-memory interface. Deinterlace and simple ISP have no extra requirements (unlike the decoder and encoder), so IMHO there's little point in pulling them out into separate files.

fair enough