Default GOPSize of -1 means only one keyframe is ever published

[playertr]

I think I've been affected by a bug that recently landed in Humble, during the upgrade from 1.0.1 to 3.0.2. This bug has resulted in only one keyframe being written by the FFMPEG transport plugin. As a result, if I instantiate a camera with:

    <param name="camera.image_raw.ffmpeg.encoder" value="hevc_nvenc" />
    <param name="camera.image_raw.ffmpeg.encoder_av_options" value="preset:ll,profile:main,crf:0" />

and then record two bags with:

ros2 bag record /camera_0/image_raw/ffmpeg -o bag1

# CTRL-C the first process

ros2 bag record /camera_0/image_raw/ffmpeg -o bag2

# CTRL-C the second process

Then the second bag will be impossible to decode because it consists only of P-frames, with no I-frames.

As far as I can tell, this is because the initialization of the encoder via parameters has recently changed as follows.

In ffmpeg_image_transport, the encoder's setGOPSize function is only called if the parameter is explicitly passed by the user:

ffmpeg_image_transport/src/ffmpeg_publisher.cpp

Line 124 in b2d028c

encoder_.setGOPSize(v.get<int>());

In ffmpeg_encoder_decoder, GOPSize_ is initialized to -1:

https://github.com/ros-misc-utilities/ffmpeg_encoder_decoder/blob/f784f6d9eac4bf3302b66e11f38bc7ccb40a4a6c/include/ffmpeg_encoder_decoder/encoder.hpp#L384

Thus, the README is incorrect:

ffmpeg_image_transport/README.md

Line 81 in b2d028c

- ``gop_size``: The number of frames between keyframes. Default: 10.

Can you advise on whether this is a legit bug, or am I misreading the documentation?

[berndpfrommer]

Thanks for reporting!
Definitely a documentation bug, if not more.
I thought that -1 would cause the encoder to pick a reasonable default.
What encoder did you use?
I wonder if I should set the gop_size default back to something > 0.

[playertr]

I used hevc_nvenc, but I also observed this with h264_nvenc. I was also surprised -- as far as I can tell, the -1 gets passed all the way to FFmpeg::AVCodecContext. The behavior for -1 is not specifically defined, so I guess it's left up to the encoder implementation how to interpret a negative GOP size.

That's as far as I got in understanding the call stack, but it's totally possible that this results in a downstream bug such as an underflow for certain implementations. I think that updating the default GOP size to 10 or 15 in ffmpeg_image_transport will probably result in the fewest surprises for existing users, but it would also be super fair to throw an error if the user leaves this value undefined.

[berndpfrommer]

I like your suggestion of logging an error message if gop_size is not specified. It's one of these settings that has a big impact on performance and particularly latency so I don't want to default it to something and later deal with questions about lagging display.

[berndpfrommer]

Changed my mind and defaulted it back to 10, but issuing a warning if not set. If you have time, please test! Thanks!