[FFmpeg-devel] [PATCH] avdevice/decklink_dec: extracting and outputing klv from vanc

[Zivkovic, Milos]

Hello,

> Please in the future send patches as in-line rather than attachments.

git send-email wasn't working for this email. I attached the patch
only after asking on #ffmpeg-devel IRC channel if it was okay.
Is simply pasting the patch contents enough?

> This is referencing the incorrect specification.

I definitely misused the word defined. What I meant is that MISB ST
0605 was used as a reference.
Comment was left mainly to have an easy to access doc if there's a
need for it, unlike SMPTE standards
that need to be paid.

> There is no support for HANC in the decklink output.  Remove the above
> unused constant.

HANC constant was left for completeness. I'll remove it.

> We already have a self-contained parser for other VANC packet types
> (i.e. teletext, captions).  I would rather just see that reused and
> have another "else if (did == 0x44 && sdid == 0x04)" simply added to
> the existing get_metadata() function than implement a completely
> different API for VANC handling.  This eliminates the requirement on
> the newer SDK and provides more consistent handling relative to the
> other VANC types currently supported.

Bumping version from 10.9.5 to 10.10 isn't such a big deal. Going
forward is not a bad thing.
Existing parser could also be rewritten using the new DeckLink API
instead of using the
legacy one, and by doing so code would become much simpler and easier
to read/maintain.
Or once libvklanc has all the features that are needed, it could
replace both legacy/new API.

> Milos, have you seen any cases where reordering is
> actually required prior to reassembly?

Not really, I was just trying to avoid any issues if it were to happen.

> Should you be tracking the PSC values to check for missing packets
> (e.g. checksum errors)?

Good point. I'll add that to the code that iterates through packets
and will discard
everything with the same MID in that case.

> Personally I would prefer "enable_klv" over "output_klv", as using the
> term "output" when describing a property of an input device could be
> confusing to some users.

Agree, I'll rename it to "enable_klv".

Thanks,
Milos