[FFmpeg-devel] [PATCH v8 3/3] avdevice/decklink_dec: export timecode with s12m side data

[Andreas Rheinhardt]

lance.lmwang at gmail.com:
> From: Limin Wang <lance.lmwang at gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> ---
>  libavdevice/decklink_dec.cpp | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index a499972..146f56f 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -42,6 +42,7 @@ extern "C" {
>  #include "libavutil/imgutils.h"
>  #include "libavutil/intreadwrite.h"
>  #include "libavutil/time.h"
> +#include "libavutil/timecode.h"
>  #include "libavutil/mathematics.h"
>  #include "libavutil/reverse.h"
>  #include "avdevice.h"
> @@ -882,6 +883,19 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                          AVDictionary* metadata_dict = NULL;
>                          int metadata_len;
>                          uint8_t* packed_metadata;
> +                        AVTimecode tcr;
> +
> +                        if (av_timecode_init_from_string(&tcr, ctx->video_st->r_frame_rate, tc, ctx) >= 0) {
> +                            uint32_t tc_data = av_timecode_get_smpte_from_framenum(&tcr, 0);
> +                            int size = sizeof(uint32_t) * 4;
> +                            uint8_t *sd = av_packet_new_side_data(&pkt, AV_PKT_DATA_S12M_TIMECODE, size);
> +
> +                            if (sd) {
> +                                AV_WL32(sd, 1); // one TC ;

Nit: why is there a space after TC?

> +                                AV_WL32(sd + 4, tc_data); // TC;

This contradicts the documentation of AV_PKT_DATA_S12M_TIMECODE, because
you are always using little endian here. But the documentation [1] does
not specify an endianness, it uses native endianness (in accordance with
what AV_FRAME_DATA_S12M_TIMECODE does). This very same patch also uses
native endianness in libavformat/dump.c.

I consider it a mistake to use native endianness for the frame side data
and an uint32_t in av_timecode_get_smpte (none of the fields cross a
byte boundary, so each entry could easily have been an uint8_t[4]), as
this will make it harder to test this side data via fate; but I also see
the advantage of using the same format and the same helper functions for
both. What do others think about this? After all, we can still change
the format for the packet side data.

- Andreas

[1]:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1593610798-21093-2-git-send-email-lance.lmwang@gmail.com/
(Is this even the latest version? I haven't paid close attention to this
patchset tbh.)