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

[Marton Balint]

On Sun, 12 Jul 2020, Andreas Rheinhardt wrote:

> 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.

I think using possibly different endianness for the same type of frame 
side data and packet side data would be a very bad idea, a consistent API 
is a lot more important. Thanks for spotting this.

Regards,
Marton