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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
