| Message ID | 1594508198-13821-1-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Sun, 12 Jul 2020, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@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 ; > + AV_WL32(sd + 4, tc_data); // TC; > + } > + } > + > if (av_dict_set(&metadata_dict, "timecode", tc, AV_DICT_DONT_STRDUP_VAL) >= 0) { > packed_metadata = av_packet_pack_dictionary(metadata_dict, &metadata_len); > av_dict_free(&metadata_dict); LGTM, thanks. Marton
lance.lmwang@gmail.com: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@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.)
On Sun, Jul 12, 2020 at 03:07:18AM +0200, Andreas Rheinhardt wrote: > lance.lmwang@gmail.com: > > From: Limin Wang <lance.lmwang@gmail.com> > > > > Signed-off-by: Limin Wang <lance.lmwang@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? Will remove it, it's not intentional. > > > + 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. good catch, at first, I think it's little endian order, and Nicolas comment uint32_t is native, so I remove the commens for the byte order. I'll change to use AV_WB32() for native write. > > 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 didn't get the SMPTE S314M:2005 specs for checking, it's not free. I think use native format is fine for local system, but if the side data is packed into or read from specific format like DV, AV_WB32() and AV_RB32() had to be used. For the fate testing, I think we can't test the side data binary only, if needed, we can test the format which use the sidedata like dv. I have add fate timecode testing for h264/hevc and haven't submit yet. But if the frame rate > 30, I got one unexpected result after map SMPTE ST 12-1:2014 side data to HEVC timecode, the frame is 6bit only(2bit for tens of frame), so to framerate > 30, it'll be divided by 2 with same timecode, but HEVC timecode frame number can use 9bit and expect the frame > 30. > > - 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.) Yes, in my local system, I change the version related for conflict. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, 12 Jul 2020, Andreas Rheinhardt wrote: > lance.lmwang@gmail.com: >> From: Limin Wang <lance.lmwang@gmail.com> >> >> Signed-off-by: Limin Wang <lance.lmwang@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
lance.lmwang@gmail.com: > On Sun, Jul 12, 2020 at 03:07:18AM +0200, Andreas Rheinhardt wrote: >> lance.lmwang@gmail.com: >>> From: Limin Wang <lance.lmwang@gmail.com> >>> >>> Signed-off-by: Limin Wang <lance.lmwang@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? > > Will remove it, it's not intentional. > >> >>> + 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. > > good catch, at first, I think it's little endian order, and Nicolas comment > uint32_t is native, so I remove the commens for the byte order. I'll change > to use AV_WB32() for native write. > AV_WB uses big-endian. Instead you could simply use an uint32_t* to write the numbers. >> >> 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 didn't get the SMPTE S314M:2005 specs for checking, it's not free. I think > use native format is fine for local system, but if the side data is packed into > or read from specific format like DV, AV_WB32() and AV_RB32() had to be used. > For the fate testing, I think we can't test the side data binary only, if needed, > we can test the format which use the sidedata like dv. The framecrc muxer calculates a checksum of every side data. There is already a special way of calculating the side-data for palettes in big-endian and it seems that we can simply reuse this (as both are based on uint32_t in native format). So it seems it won't be much of a problem. - Andreas
On Sun, Jul 12, 2020 at 06:03:04PM +0200, Andreas Rheinhardt wrote: > lance.lmwang@gmail.com: > > On Sun, Jul 12, 2020 at 03:07:18AM +0200, Andreas Rheinhardt wrote: > >> lance.lmwang@gmail.com: > >>> From: Limin Wang <lance.lmwang@gmail.com> > >>> > >>> Signed-off-by: Limin Wang <lance.lmwang@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? > > > > Will remove it, it's not intentional. > > > >> > >>> + 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. > > > > good catch, at first, I think it's little endian order, and Nicolas comment > > uint32_t is native, so I remove the commens for the byte order. I'll change > > to use AV_WB32() for native write. > > > > AV_WB uses big-endian. Instead you could simply use an uint32_t* to > write the numbers. OK, I'll change it. > > >> > >> 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 didn't get the SMPTE S314M:2005 specs for checking, it's not free. I think > > use native format is fine for local system, but if the side data is packed into > > or read from specific format like DV, AV_WB32() and AV_RB32() had to be used. > > For the fate testing, I think we can't test the side data binary only, if needed, > > we can test the format which use the sidedata like dv. > > The framecrc muxer calculates a checksum of every side data. There is > already a special way of calculating the side-data for palettes in > big-endian and it seems that we can simply reuse this (as both are based > on uint32_t in native format). So it seems it won't be much of a problem. > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, Jul 12, 2020 at 6:16 AM <lance.lmwang@gmail.com> wrote: > I have add fate timecode testing for h264/hevc and haven't submit yet. But if > the frame rate > 30, I got one unexpected result after map SMPTE ST 12-1:2014 > side data to HEVC timecode, the frame is 6bit only(2bit for tens of frame), > so to framerate > 30, it'll be divided by 2 with same timecode, but HEVC timecode > frame number can use 9bit and expect the frame > 30. You should still divide it by 2, even though HEVC supports more bits for the frame number. This is to be consistent with SMPTE 12-1 timecode, and downstream muxers/outputs aren't going to know whether the upstream decoder was H.264 or HEVC. On the encode side, you can multiple by 2 and use the field bit to determine whether to then add 1. See SMPTE ST 12-1:2014 Sec 12.2. There's an argument that we should add support for higher framerates to support SMPTE 12-3 (which supports fps > 60), but that should be a different side data field. Right now the most important thing is that the behavior be consistent across decoders. Devin
On Mon, Jul 13, 2020 at 10:24:11AM -0400, Devin Heitmueller wrote: > On Sun, Jul 12, 2020 at 6:16 AM <lance.lmwang@gmail.com> wrote: > > I have add fate timecode testing for h264/hevc and haven't submit yet. But if > > the frame rate > 30, I got one unexpected result after map SMPTE ST 12-1:2014 > > side data to HEVC timecode, the frame is 6bit only(2bit for tens of frame), > > so to framerate > 30, it'll be divided by 2 with same timecode, but HEVC timecode > > frame number can use 9bit and expect the frame > 30. > > You should still divide it by 2, even though HEVC supports more bits > for the frame number. This is to be consistent with SMPTE 12-1 > timecode, and downstream muxers/outputs aren't going to know whether > the upstream decoder was H.264 or HEVC. On the encode side, you can > multiple by 2 and use the field bit to determine whether to then add > 1. See SMPTE ST 12-1:2014 Sec 12.2. Thanks for the comments, it's helpful, I'll try to fix it on encoder side. > > There's an argument that we should add support for higher framerates > to support SMPTE 12-3 (which supports fps > 60), but that should be a > different side data field. Right now the most important thing is that > the behavior be consistent across decoders. > > Devin > > -- > Devin Heitmueller, Senior Software Engineer > LTN Global Communications > o: +1 (301) 363-1001 > w: https://ltnglobal.com e: devin.heitmueller@ltnglobal.com > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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 ; + AV_WL32(sd + 4, tc_data); // TC; + } + } + if (av_dict_set(&metadata_dict, "timecode", tc, AV_DICT_DONT_STRDUP_VAL) >= 0) { packed_metadata = av_packet_pack_dictionary(metadata_dict, &metadata_len); av_dict_free(&metadata_dict);