| Message ID | 20170630220536.19396-1-cus@passwd.hu |
|---|---|
| State | Superseded |
| Headers | show |
On 6/30/2017 5:05 PM, Marton Balint wrote: > This also add supports for 4K DeckLink cards because they always output the > ancillary data in 10-bit. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > doc/indevs.texi | 4 ++-- > libavdevice/decklink_dec.cpp | 56 +++++++++++++++++++++++++++++++------------- > 2 files changed, 42 insertions(+), 18 deletions(-) > > diff --git a/doc/indevs.texi b/doc/indevs.texi > index 51c304f3ec..330617a7c9 100644 > --- a/doc/indevs.texi > +++ b/doc/indevs.texi > @@ -250,8 +250,8 @@ specifically lines 6 to 22, and lines 318 to 335. Line 6 is the LSB in the mask. > Selected lines which do not contain teletext information will be ignored. You > can use the special @option{all} constant to select all possible lines, or > @option{standard} to skip lines 6, 318 and 319, which are not compatible with all > -receivers. Capturing teletext only works for SD PAL sources in 8 bit mode. > -To use this option, ffmpeg needs to be compiled with @code{--enable-libzvbi}. > +receivers. Capturing teletext only works for SD PAL sources. To use this > +option, ffmpeg needs to be compiled with @code{--enable-libzvbi}. > > @item channels > Defines number of audio channels to capture. Must be @samp{2}, @samp{8} or @samp{16}. > diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp > index 39974e3ff4..88af01be70 100644 > --- a/libavdevice/decklink_dec.cpp > +++ b/libavdevice/decklink_dec.cpp > @@ -54,21 +54,40 @@ static uint8_t calc_parity_and_line_offset(int line) > return ret; > } > > -int teletext_data_unit_from_vbi_data(int line, uint8_t *src, uint8_t *tgt) > +static void fill_data_unit_head(int line, uint8_t *tgt) > +{ > + tgt[0] = 0x02; // data_unit_id > + tgt[1] = 0x2c; // data_unit_length > + tgt[2] = calc_parity_and_line_offset(line); // field_parity, line_offset > + tgt[3] = 0xe4; // framing code > +} > + > +static uint8_t* teletext_data_unit_from_vbi_data(int line, uint8_t *src, uint8_t *tgt, vbi_pixfmt fmt) > { > vbi_bit_slicer slicer; > > - vbi_bit_slicer_init(&slicer, 720, 13500000, 6937500, 6937500, 0x00aaaae4, 0xffff, 18, 6, 42 * 8, VBI_MODULATION_NRZ_MSB, VBI_PIXFMT_UYVY); > + vbi_bit_slicer_init(&slicer, 720, 13500000, 6937500, 6937500, 0x00aaaae4, 0xffff, 18, 6, 42 * 8, VBI_MODULATION_NRZ_MSB, fmt); > > if (vbi_bit_slice(&slicer, src, tgt + 4) == FALSE) > - return -1; > + return tgt; > > - tgt[0] = 0x02; // data_unit_id > - tgt[1] = 0x2c; // data_unit_length > - tgt[2] = calc_parity_and_line_offset(line); // field_parity, line_offset > - tgt[3] = 0xe4; // framing code > + fill_data_unit_head(line, tgt); > > - return 0; > + return tgt + 46; > +} > + > +static uint8_t* teletext_data_unit_from_vbi_data_10bit(int line, uint8_t *src, uint8_t *tgt) > +{ > + uint8_t y[720]; > + uint8_t *py = y; > + uint8_t *pend = y + 720; > + while (py < pend) { > + *py++ = (src[1] >> 4) + ((src[2] & 15) << 4); > + *py++ = (src[4] >> 2) + ((src[5] & 3 ) << 6); > + *py++ = (src[6] >> 6) + ((src[7] & 63) << 2); > + src += 8; > + } It would seem that the purpose of this code is to convert between the Blackmagic 10-bit format (v210, according to the Blackmagic documentation) and YUV420 (which is presumably easier than converting to UYVY). Why not use a color converter for this? Looking through the ffmpeg code base, I see that v210 is handled using a decoder/encoder and not as a pixel format, which seems odd, but maybe there is some way to take advantage of that support instead of doing it yourself. At the very least, it seems reasonable to add a comment explaining what this code is doing. Also, I understand that this is being done because libzvbi doesn't support the v210 pixel format (which would be useful to explain in a comment), but I have to wonder if the conversion between the 10-bit v210 pixel format and the 8-bit YUV420 pixel format will result in a loss of data that is relevant for teletext. If you've accommodated that possibility, it would be good to describe in a comment. > + return teletext_data_unit_from_vbi_data(line, y, tgt, VBI_PIXFMT_YUV420); > } > #endif > > @@ -359,7 +378,7 @@ HRESULT decklink_input_callback::VideoInputFrameArrived( > //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size, pkt.pts); > > #if CONFIG_LIBZVBI > - if (!no_video && ctx->teletext_lines && videoFrame->GetPixelFormat() == bmdFormat8BitYUV && videoFrame->GetWidth() == 720) { > + if (!no_video && ctx->teletext_lines) { > IDeckLinkVideoFrameAncillary *vanc; > AVPacket txt_pkt; > uint8_t txt_buf0[1611]; // max 35 * 46 bytes decoded teletext lines + 1 byte data_identifier > @@ -368,16 +387,21 @@ HRESULT decklink_input_callback::VideoInputFrameArrived( > if (videoFrame->GetAncillaryData(&vanc) == S_OK) { > int i; > int64_t line_mask = 1; > + BMDPixelFormat vanc_format = vanc->GetPixelFormat(); > txt_buf[0] = 0x10; // data_identifier - EBU_data > txt_buf++; > - for (i = 6; i < 336; i++, line_mask <<= 1) { > - uint8_t *buf; > - if ((ctx->teletext_lines & line_mask) && vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) { > - if (teletext_data_unit_from_vbi_data(i, buf, txt_buf) >= 0) > - txt_buf += 46; > + if (videoFrame->GetWidth() == 720 && (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) { According to the documentation in indevs.texi, the teletext support only works for SD PAL. Yet, this check doesn't isolate things just to SD PAL. I know that this issue was preexisting (although you have moved the check to a different place in the code), but if you want to restrict this code to SD PAL, then the best way is to check if the BMDDisplayMode is bmdModePAL. If you want to base things on the width/height, you could check for a width of 720 and a height of 576, although this could indicate either bmdModePAL or bmdModePALp (although there may be no Blackmagic devices that support bmdModePALp on input). Just checking for a width of 720 includes NTSC, NTSCp, PAL, and PALp. > + for (i = 6; i < 336; i++, line_mask <<= 1) { > + uint8_t *buf; > + if ((ctx->teletext_lines & line_mask) && vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) { > + if (vanc_format == bmdFormat8BitYUV) > + txt_buf = teletext_data_unit_from_vbi_data(i, buf, txt_buf, VBI_PIXFMT_UYVY); > + else > + txt_buf = teletext_data_unit_from_vbi_data_10bit(i, buf, txt_buf); Since you already know if the format is 10-bit or 8-bit prior to entering the for loop, putting the check here may result in an unnecessary branch every loop iteration, depending on how the compiler optimizes the code. So, for efficiency, it would make more sense to move the if check to earlier, prior to executing the for loop, but this will end up causing you to effectively duplicate the for loop code the way it is currently written. > + } > + if (i == 22) > + i = 317; > } > - if (i == 22) > - i = 317; > } > vanc->Release(); > if (txt_buf - txt_buf0 > 1) { > Overall, looks good. I'll review the second patch in a bit. Aaron Levinson
On Mon, 3 Jul 2017, Aaron Levinson wrote: >> +static uint8_t* teletext_data_unit_from_vbi_data_10bit(int line, uint8_t > *src, uint8_t *tgt) >> +{ >> + uint8_t y[720]; >> + uint8_t *py = y; >> + uint8_t *pend = y + 720; >> + while (py < pend) { >> + *py++ = (src[1] >> 4) + ((src[2] & 15) << 4); >> + *py++ = (src[4] >> 2) + ((src[5] & 3 ) << 6); >> + *py++ = (src[6] >> 6) + ((src[7] & 63) << 2); >> + src += 8; >> + } > > It would seem that the purpose of this code is to convert between the > Blackmagic 10-bit format (v210, according to the Blackmagic > documentation) and YUV420 (which is presumably easier than converting to > UYVY). Why not use a color converter for this? Because I only need the luma component and only in 8 bit, so getting anything else would be a waste of resources. Also the code is short enough and more readable than factorizing something from v210dec. > Looking through the > ffmpeg code base, I see that v210 is handled using a decoder/encoder and > not as a pixel format, which seems odd, but maybe there is some way to > take advantage of that support instead of doing it yourself. At the > very least, it seems reasonable to add a comment explaining what this > code is doing. ok for the comment. > > Also, I understand that this is being done because libzvbi doesn't > support the v210 pixel format (which would be useful to explain in a > comment), but I have to wonder if the conversion between the 10-bit v210 > pixel format and the 8-bit YUV420 pixel format will result in a loss of > data that is relevant for teletext. If you've accommodated that > possibility, it would be good to describe in a comment. I understand your concern, but for teletext you decode roughly 0.5 bit of information from a 8 bit value. So even using 8bit is a lot more than what you need. I will add a comment about this. >> #if CONFIG_LIBZVBI >> - if (!no_video && ctx->teletext_lines && > videoFrame->GetPixelFormat() == bmdFormat8BitYUV && videoFrame->GetWidth() == > 720) { >> + if (!no_video && ctx->teletext_lines) { >> IDeckLinkVideoFrameAncillary *vanc; >> AVPacket txt_pkt; >> uint8_t txt_buf0[1611]; // max 35 * 46 bytes decoded teletext > lines + 1 byte data_identifier >> @@ -368,16 +387,21 @@ HRESULT > decklink_input_callback::VideoInputFrameArrived( >> if (videoFrame->GetAncillaryData(&vanc) == S_OK) { >> int i; >> int64_t line_mask = 1; >> + BMDPixelFormat vanc_format = vanc->GetPixelFormat(); >> txt_buf[0] = 0x10; // data_identifier - EBU_data >> txt_buf++; >> - for (i = 6; i < 336; i++, line_mask <<= 1) { >> - uint8_t *buf; >> - if ((ctx->teletext_lines & line_mask) && > vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) { >> - if (teletext_data_unit_from_vbi_data(i, buf, > txt_buf) >= 0) >> - txt_buf += 46; >> + if (videoFrame->GetWidth() == 720 && (vanc_format == > bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) { > > According to the documentation in indevs.texi, the teletext support only > works for SD PAL. Yet, this check doesn't isolate things just to SD > PAL. I know that this issue was preexisting (although you have moved > the check to a different place in the code), but if you want to restrict > this code to SD PAL, then the best way is to check if the BMDDisplayMode > is bmdModePAL. If you want to base things on the width/height, you > could check for a width of 720 and a height of 576, although this could > indicate either bmdModePAL or bmdModePALp (although there may be no > Blackmagic devices that support bmdModePALp on input). Just checking > for a width of 720 includes NTSC, NTSCp, PAL, and PALp. Ok. > >> + for (i = 6; i < 336; i++, line_mask <<= 1) { >> + uint8_t *buf; >> + if ((ctx->teletext_lines & line_mask) && > vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) { >> + if (vanc_format == bmdFormat8BitYUV) >> + txt_buf = > teletext_data_unit_from_vbi_data(i, buf, txt_buf, VBI_PIXFMT_UYVY); >> + else >> + txt_buf = > teletext_data_unit_from_vbi_data_10bit(i, buf, txt_buf); > > Since you already know if the format is 10-bit or 8-bit prior to > entering the for loop, putting the check here may result in an > unnecessary branch every loop iteration, depending on how the compiler > optimizes the code. So, for efficiency, it would make more sense to > move the if check to earlier, prior to executing the for loop, but this > will end up causing you to effectively duplicate the for loop code the > way it is currently written. > The overhead is negligable (35 if-s per frame) and I find this more readable than duplicating the whole loop. >> + } >> + if (i == 22) >> + i = 317; >> } >> - if (i == 22) >> - i = 317; >> } >> vanc->Release(); >> if (txt_buf - txt_buf0 > 1) { >> > > Overall, looks good. I'll review the second patch in a bit. Thanks, I will send a v2. Regards, Marton
diff --git a/doc/indevs.texi b/doc/indevs.texi index 51c304f3ec..330617a7c9 100644 --- a/doc/indevs.texi +++ b/doc/indevs.texi @@ -250,8 +250,8 @@ specifically lines 6 to 22, and lines 318 to 335. Line 6 is the LSB in the mask. Selected lines which do not contain teletext information will be ignored. You can use the special @option{all} constant to select all possible lines, or @option{standard} to skip lines 6, 318 and 319, which are not compatible with all -receivers. Capturing teletext only works for SD PAL sources in 8 bit mode. -To use this option, ffmpeg needs to be compiled with @code{--enable-libzvbi}. +receivers. Capturing teletext only works for SD PAL sources. To use this +option, ffmpeg needs to be compiled with @code{--enable-libzvbi}. @item channels Defines number of audio channels to capture. Must be @samp{2}, @samp{8} or @samp{16}. diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp index 39974e3ff4..88af01be70 100644 --- a/libavdevice/decklink_dec.cpp +++ b/libavdevice/decklink_dec.cpp @@ -54,21 +54,40 @@ static uint8_t calc_parity_and_line_offset(int line) return ret; } -int teletext_data_unit_from_vbi_data(int line, uint8_t *src, uint8_t *tgt) +static void fill_data_unit_head(int line, uint8_t *tgt) +{ + tgt[0] = 0x02; // data_unit_id + tgt[1] = 0x2c; // data_unit_length + tgt[2] = calc_parity_and_line_offset(line); // field_parity, line_offset + tgt[3] = 0xe4; // framing code +} + +static uint8_t* teletext_data_unit_from_vbi_data(int line, uint8_t *src, uint8_t *tgt, vbi_pixfmt fmt) { vbi_bit_slicer slicer; - vbi_bit_slicer_init(&slicer, 720, 13500000, 6937500, 6937500, 0x00aaaae4, 0xffff, 18, 6, 42 * 8, VBI_MODULATION_NRZ_MSB, VBI_PIXFMT_UYVY); + vbi_bit_slicer_init(&slicer, 720, 13500000, 6937500, 6937500, 0x00aaaae4, 0xffff, 18, 6, 42 * 8, VBI_MODULATION_NRZ_MSB, fmt); if (vbi_bit_slice(&slicer, src, tgt + 4) == FALSE) - return -1; + return tgt; - tgt[0] = 0x02; // data_unit_id - tgt[1] = 0x2c; // data_unit_length - tgt[2] = calc_parity_and_line_offset(line); // field_parity, line_offset - tgt[3] = 0xe4; // framing code + fill_data_unit_head(line, tgt); - return 0; + return tgt + 46; +} + +static uint8_t* teletext_data_unit_from_vbi_data_10bit(int line, uint8_t *src, uint8_t *tgt) +{ + uint8_t y[720]; + uint8_t *py = y; + uint8_t *pend = y + 720; + while (py < pend) { + *py++ = (src[1] >> 4) + ((src[2] & 15) << 4); + *py++ = (src[4] >> 2) + ((src[5] & 3 ) << 6); + *py++ = (src[6] >> 6) + ((src[7] & 63) << 2); + src += 8; + } + return teletext_data_unit_from_vbi_data(line, y, tgt, VBI_PIXFMT_YUV420); } #endif @@ -359,7 +378,7 @@ HRESULT decklink_input_callback::VideoInputFrameArrived( //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size, pkt.pts); #if CONFIG_LIBZVBI - if (!no_video && ctx->teletext_lines && videoFrame->GetPixelFormat() == bmdFormat8BitYUV && videoFrame->GetWidth() == 720) { + if (!no_video && ctx->teletext_lines) { IDeckLinkVideoFrameAncillary *vanc; AVPacket txt_pkt; uint8_t txt_buf0[1611]; // max 35 * 46 bytes decoded teletext lines + 1 byte data_identifier @@ -368,16 +387,21 @@ HRESULT decklink_input_callback::VideoInputFrameArrived( if (videoFrame->GetAncillaryData(&vanc) == S_OK) { int i; int64_t line_mask = 1; + BMDPixelFormat vanc_format = vanc->GetPixelFormat(); txt_buf[0] = 0x10; // data_identifier - EBU_data txt_buf++; - for (i = 6; i < 336; i++, line_mask <<= 1) { - uint8_t *buf; - if ((ctx->teletext_lines & line_mask) && vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) { - if (teletext_data_unit_from_vbi_data(i, buf, txt_buf) >= 0) - txt_buf += 46; + if (videoFrame->GetWidth() == 720 && (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) { + for (i = 6; i < 336; i++, line_mask <<= 1) { + uint8_t *buf; + if ((ctx->teletext_lines & line_mask) && vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) { + if (vanc_format == bmdFormat8BitYUV) + txt_buf = teletext_data_unit_from_vbi_data(i, buf, txt_buf, VBI_PIXFMT_UYVY); + else + txt_buf = teletext_data_unit_from_vbi_data_10bit(i, buf, txt_buf); + } + if (i == 22) + i = 317; } - if (i == 22) - i = 317; } vanc->Release(); if (txt_buf - txt_buf0 > 1) {
This also add supports for 4K DeckLink cards because they always output the ancillary data in 10-bit. Signed-off-by: Marton Balint <cus@passwd.hu> --- doc/indevs.texi | 4 ++-- libavdevice/decklink_dec.cpp | 56 +++++++++++++++++++++++++++++++------------- 2 files changed, 42 insertions(+), 18 deletions(-)