add sps timings

[dhamp]

add use MMAL_PARAMETER_VIDEO_ENCODE_SPS_TIMING for correct setup fps in h.264

[JamesH65]

Not sure, but shouldn't that be conditional? ie specified on the command line? Rather than just set as it presumably changes it for everyone. @6by9?

[6by9]

Sorry, I'm very reluctant to change the default behaviour. Adding a command line flag to enable sps timings would be acceptable.

When the SPS_TIMINGs parameter got added via raspberrypi/firmware#245 we couldn't get hold of the relevant H264 codec experts to confirm that setting it by default could be guaranteed not to cause problems elsewhere, hence why it wasn't enabled by default.

There are a couple of other foibles.

• The camera always delivers frames at intervals that are a multiple of the line length (generally around 18us, but dependent on sensor and mode), so the frame rate is never dead on the requested frame rate, and therefore the value encoded is wrong. It also can't handle frame drops. Frame timestamps are the only guaranteed timing.
• The video_encode framework currently only handles integer values for framerate, and as it uses the value in bitrate control it's not a trivial thing to switch to fixed point and be 100% certain of avoiding regressions. OK raspivid doesn't allow setting fractional frame rates, but it is taken as the reference code. Ah, I can even find the issue - h264 encoder does not support fractional frame rates in SPS VUI firmware#1007.

Minor nitpick - mmal_port_parameter_set_boolean takes a MMAL_BOOL_T (ie MMAL_TRUE, or MMAL_FALSE). 1 is not a MMAL_BOOL_T.

[6by9]

And one other minor nit pick from an admin side. The commit is authored by one Eugene Petrov with an email address not apparently linked to any github account. Do you have his permission to use this patch? We don't insist on signed-off-by tags, but normally expect PRs to be coming from the author and therefore the PR is the permission to use it.

[dhamp]

Add option for cli
Group some settings in one if valid only for h.264

[6by9]

Thanks. I'm happy with that.
@JamesH65 any comments from you?

[JamesH65]

Apart from one non-Allman bracketing on line 2213 , LGTM.

[dhamp]

Any other comments?

[JamesH65]

Was there any comment on this #488 (comment)?

If all is well, then LGTM and will merge.

[6by9]

Was there any comment on this #488 (comment)?

Commit email address has been updated and now comes back to dhamp, so I'm happy on that.
White space has crept in on line 2242, but I'm not fussing on that.

Just compile testing and will then squash and merge.

[6by9]

Ooh, glad I did.

userland/host_applications/linux/apps/raspicam/RaspiVid.c: In function ‘dump_status’:
/nfs/pi2_stretch_kms/home/pi/userland/host_applications/linux/apps/raspicam/RaspiVid.c:511:4: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘char *’ [-Wformat=]
    fprintf(stderr, "H264 Fill SPS Timings %d\n", state->addSPSTiming ? "Yes" : "No");
    ^

%s required, not %d

[dhamp]

fprintf fixed

[dhamp]

merge commits into one or leave as is?

[6by9]

merge commits into one or leave as is?

We get the choice of "Create a merge commit", "Squash and merge", or "rebase and merge" when accepting the PR. It's easy enough for us to squash and merge, so no need to worry your end.
I'm rebuilding just to check for any other gremlins (not expecting any now).

[6by9]

Merged. Thanks.