On Mon, 3 Jul 2017, Aaron Levinson wrote:
+
+This option is a bitmask of the SD VBI lines captured, specifically lines
6 to
Probably should be "SD PAL VBI lines" to make it clear that NTSC is not
supported.
ok.
+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.
+
+For SD sources ffmpeg needs to be compiled with @code{--enable-libzvbi}.
For HD
+sources on older (pre-4K) DeckLink card models you have to capture in 10
bit
+mode.
Would be good to indicate that the bit mask is ignored for HD sources.
Actually it is not. Any HD line can contain an OP47 packet referencing any
SD line, the bitmask is checked against the decoded source line number
from OP47.
The documentation indicates that both SD PAL and HD sources are
supported, but an examination of the changes indicates that only some HD
sources are supported. Specifically, for HD sources to work, it expects
a width of 1920. This would cover both 1080i and 1080p, but it doesn't
cover 720p, with is also an HD video mode. My guess is that the code
has probably only been tested with 1080i as well, and in that case, it
would make sense to only specify 1080i in the documentation. Further,
since the original code only supports SD PAL, I would suspect that the
latest code has only been tested using "PAL" frame rates at 1080i, i.e.
1080i50. If it is unclear if the code will also work with 1080i59.94
and 1080i60, then it would be best to only support 1080i50.
1080i/p/59.94/60 should work all the same. For 720p different VANC lines
might be needed. I will change the HD references to "Full HD" so it will
be more clear.
Also, should have a comma after "For SD sources" and after "For HD sources".
Ok.
+static uint8_t* teletext_data_unit_from_vanc_data(uint8_t *src, uint8_t
*tgt, int64_t wanted_lines)
+{
+ int y[1920];
+ int *py = y;
+ int *pend = y + 1920;
+ while (py < pend) {
+ *py++ = (src[1] >> 2) + ((src[2] & 15) << 6);
+ *py++ = src[4] + ((src[5] & 3) << 8);
+ *py++ = (src[6] >> 4) + ((src[7] & 63) << 4);
+ src += 8;
+ }
My comments from the last review pertain to this code. Plus, now this
code is duplicated, so it would make sense to consolidate it to a new
function.
Not exaclty the same, because I need the 10 bit data here. Only thing
I could do is use some DEFINE magic to factorize this.
+ py = y;
+ while (py < pend - 6) {
+ if (py[0] == 0 && py[1] == 0x3ff && py[2] == 0x3ff) {
// ancilliary data flag
"ancilliary" -> "ancillary" in the comment
ok.
-#if CONFIG_LIBZVBI
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
+ uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext
lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
It is a little hard to follow this code, but it would seem that a max of
1611 or 1920 bytes would be needed (so 1920), not 1611 + 1920 bytes.
Not exactly, in the HD case 1611 bytes are reserved for existing decoded
packets, 1920 bytes are reserved for the teletext decoded from the next
VANC line, which might contain multiple teletext lines.
uint8_t *txt_buf = txt_buf0;
if (videoFrame->GetAncillaryData(&vanc) == S_OK) {
@@ -390,6 +478,7 @@ HRESULT
decklink_input_callback::VideoInputFrameArrived(
BMDPixelFormat vanc_format = vanc->GetPixelFormat();
txt_buf[0] = 0x10; // data_identifier - EBU_data
txt_buf++;
+#if CONFIG_LIBZVBI
if (videoFrame->GetWidth() == 720 && (vanc_format ==
bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
for (i = 6; i < 336; i++, line_mask <<= 1) {
uint8_t *buf;
@@ -403,6 +492,20 @@ HRESULT
decklink_input_callback::VideoInputFrameArrived(
i = 317;
}
}
+#endif
+ if (videoFrame->GetWidth() == 1920 && vanc_format ==
bmdFormat10BitYUV) {
As written, this would support all 1080p and 1080i video modes supported
by DeckLink. I would suspect that is not the desired behavior. See the
relevant comments from my review of the first patch.
It is in this case.
+ for (i = 8; i < 584; i++) {
+ uint8_t *buf;
+ if (vanc->GetBufferForVerticalBlankingLine(i,
(void**)&buf) == S_OK)
+ txt_buf =
teletext_data_unit_from_vanc_data(buf, txt_buf, ctx->teletext_lines);
+ if (i == 20)
+ i = 569;
+ if (txt_buf - txt_buf0 > 1611) {
Since you added 1920 bytes to support OP47, it would seem that all 1920
bytes may be relevant. So, this above check may be wrong.
No, I check here that I still have 1920 bytes available in the buffer.
+ av_log(avctx, AV_LOG_ERROR, "Too many OP47
teletext packets.\n");
+ break;
+ }
+ }
+ }
vanc->Release();
I will send a v2 for this as well.
Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
