Hello Derek,
Comments inline.
>>
>> + afd[0] = pkt->hdr.payload[0] >> 3;
>> + if (av_packet_add_side_data(cb_ctx->pkt, AV_PKT_DATA_AFD, afd, 1) < 0)
>> + av_free(afd);
>
> Is there a reason we shouldn't fail hard here?
Not really. The parser will log an error if the callback returns a nonzero
value, but beyond the return value isn’t actively used. That said, no
objection to having it return -1 for clarity.
>
>> +static struct klvanc_callbacks_s callbacks =
>> +{
>> + .afd = cb_AFD,
>> + .eia_708b = cb_EIA_708B,
>> + .eia_608 = NULL,
>> + .scte_104 = NULL,
>> + .all = NULL,
>> + .kl_i64le_counter = NULL,
>> +};
>
> I thought C++ didn't have designated initializers? Maybe my C++ is rusty.
Clang didn’t complain, and g++ only complains if you put them in a non-default
order (i.e. "non-trivial designated initializers not supported"). The
designated initializers improve readability but aren’t required (since already
put the items in the default order). If there’s a portability concern then I
can get rid of them.
>
> Same for other occurrences.
I’m sorry, but what other occurrences? I don’t see any other instances in this
patch where designated initializers are used — or did I misunderstand your
comment?
>
>> + /* Convert the vanc line from V210 to CrCB422, then vanc parse it */
>> +
>> + /* We need two kinds of type pointers into the source vbi buffer */
>> + /* TODO: What the hell is this, two ptrs? */
>> + const uint32_t *src = (const uint32_t *)buf;
>
> Is buf guaranteed to be properly aligned for this, or will cause aliasing
> problems?
Hmm, good question. The start of each line will always be aligned on a 48 byte
boundary as a result of how the decklink module manages it’s buffers, but I
agree that this block of code is a bit messy and needs some cleanup (hence the
TODO).
I suspect the original routine was cribbed from OBE (with portions derived from
ffmpeg’s v210dec), and the assembly version of the same function probably isn’t
as forgiving (although libklvanc doesn’t provide an assembly implementation as
this routine isn’t particularly performance sensitive).
>
>> + vanc_ctx->callback_context = &cb_ctx;
>> + int ret = klvanc_packet_parse(vanc_ctx, lineNr, decoded_words,
>> sizeof(decoded_words) / (sizeof(unsigned short)));
>
> Nobody should be typing 'short' in any C/C++ code in 2017..
Will fix.
>
>> + if (ret < 0) {
>> + /* No VANC on this line */
>> + }
>
> Huh?
The parser takes in the complete VANC lines, but it’s possible that those lines
are blank and don’t contain any actual VANC packets. That said, you’re right -
a negative return should be treated as an error and the comment in question
should only occur if the return value is zero (a positive return value is the
number of packets parsed). Will fix.
>
>> +#if CONFIG_LIBKLVANC
>> + klvanc_handle_line(avctx, ctx->vanc_ctx,
>> + buf, videoFrame->GetWidth(),
>> i, &pkt);
>> +#else
>
> No error checking possible?
Will fix.
>
>> }
>> +
>> vanc->Release();
>
> Stray change.
Will fix.
>
>> +#if CONFIG_LIBKLVANC
>> + if (klvanc_context_create(&ctx->vanc_ctx) < 0) {
>> + av_log(avctx, AV_LOG_ERROR, "Cannot create VANC library context\n");
>> + } else {
>> + ctx->vanc_ctx->verbose = 0;
>> + ctx->vanc_ctx->callbacks = &callbacks;
>> + }
>> +#endif
>
> Should fail hard, no?
Will fix.
Thanks for reviewing,
Devin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
