| Message ID | 20171229181230.99473-3-dheitmueller@ltnglobal.com |
|---|---|
| State | Superseded |
| Headers | show |
2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: > + /* FIXME: Should really rely on the coded_width but seems like that > + is not accessible to libavdevice outputs */ > + if ((st->codecpar->width == 1280 && st->codecpar->height == 720) || > + (st->codecpar->width == 1920 && st->codecpar->height == 1080)) > + pkt->aspectRatio = ASPECT_16x9; > + else > + pkt->aspectRatio = ASPECT_4x3; I most likely won't use this (and I have never seen a decklink card) so please feel free to ignore: Similar code has caused some trouble with mxf files, is there really no saner solution? Like comparing what the actual aspect ratio is more similar to? Is SAR really always 1 for decklink? ("All the world's a VAX.") Carl Eugen
> On Dec 29, 2017, at 3:48 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > 2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: > >> + /* FIXME: Should really rely on the coded_width but seems like that >> + is not accessible to libavdevice outputs */ >> + if ((st->codecpar->width == 1280 && st->codecpar->height == 720) || >> + (st->codecpar->width == 1920 && st->codecpar->height == 1080)) >> + pkt->aspectRatio = ASPECT_16x9; >> + else >> + pkt->aspectRatio = ASPECT_4x3; > > I most likely won't use this (and I have never seen a decklink card) > so please feel free to ignore: > Similar code has caused some trouble with mxf files, is there > really no saner solution? Like comparing what the actual aspect > ratio is more similar to? Is SAR really always 1 for decklink? > ("All the world's a VAX.") So this is definitely a confusing block of code, and you aren’t the first one to ask about it (there were questions in the last round of review as well). The aspect ratio referred to here is actually of the original coded video - not how it’s supposed to be displayed. Hence, for example, 720x480 in widescreen with a non-square PAR would still have the aspect ratio set to 4x3, since that particular field describes the coded video (i.e. *not* how it’s supposed to be rendered). Devin
2017-12-29 22:02 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: > >> On Dec 29, 2017, at 3:48 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> >> 2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: >> >>> + /* FIXME: Should really rely on the coded_width but seems like that >>> + is not accessible to libavdevice outputs */ >>> + if ((st->codecpar->width == 1280 && st->codecpar->height == 720) || >>> + (st->codecpar->width == 1920 && st->codecpar->height == 1080)) >>> + pkt->aspectRatio = ASPECT_16x9; >>> + else >>> + pkt->aspectRatio = ASPECT_4x3; >> >> I most likely won't use this (and I have never seen a decklink card) >> so please feel free to ignore: >> Similar code has caused some trouble with mxf files, is there >> really no saner solution? Like comparing what the actual aspect >> ratio is more similar to? Is SAR really always 1 for decklink? >> ("All the world's a VAX.") > > So this is definitely a confusing block of code, and you aren’t the first one to > ask about it (there were questions in the last round of review as well). The > aspect ratio referred to here is actually of the original coded video - not how > it’s supposed to be displayed. Hence, for example, 720x480 in widescreen > with a non-square PAR would still have the aspect ratio set to 4x3, since > that particular field describes the coded video (i.e. *not* how it’s supposed > to be rendered). And a resolution of 1024x576 does not exist for decklink? What about 1920x1088? Carl Eugen
> On Dec 29, 2017, at 4:09 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > 2017-12-29 22:02 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: >> >>> On Dec 29, 2017, at 3:48 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> >>> 2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: >>> >>>> + /* FIXME: Should really rely on the coded_width but seems like that >>>> + is not accessible to libavdevice outputs */ >>>> + if ((st->codecpar->width == 1280 && st->codecpar->height == 720) || >>>> + (st->codecpar->width == 1920 && st->codecpar->height == 1080)) >>>> + pkt->aspectRatio = ASPECT_16x9; >>>> + else >>>> + pkt->aspectRatio = ASPECT_4x3; >>> >>> I most likely won't use this (and I have never seen a decklink card) >>> so please feel free to ignore: >>> Similar code has caused some trouble with mxf files, is there >>> really no saner solution? Like comparing what the actual aspect >>> ratio is more similar to? Is SAR really always 1 for decklink? >>> ("All the world's a VAX.") >> >> So this is definitely a confusing block of code, and you aren’t the first one to >> ask about it (there were questions in the last round of review as well). The >> aspect ratio referred to here is actually of the original coded video - not how >> it’s supposed to be displayed. Hence, for example, 720x480 in widescreen >> with a non-square PAR would still have the aspect ratio set to 4x3, since >> that particular field describes the coded video (i.e. *not* how it’s supposed >> to be rendered). > > And a resolution of 1024x576 does not exist for decklink? > > What about 1920x1088? > The Blackmagic cards have a fairly constrained set of modes which are supported (see the definition of BMDDisplayMode in the Decklink SDK documentation). Neither of the modes you mentioned are available for output, although I’m not sure what st->codecpar->width would be set to if swscale is used between the decoder and the output. Bear in mind, I’m not particularly happy with how that block of code is done either (which is why I marked it with a FIXME), but it’s probably good enough for 95% of use cases. Devin
On 12/29/2017 1:20 PM, Devin Heitmueller wrote: > >> On Dec 29, 2017, at 4:09 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> >> 2017-12-29 22:02 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: >>> >>>> On Dec 29, 2017, at 3:48 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> >>>> 2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>: >>>> >>>>> + /* FIXME: Should really rely on the coded_width but seems like that >>>>> + is not accessible to libavdevice outputs */ >>>>> + if ((st->codecpar->width == 1280 && st->codecpar->height == 720) || >>>>> + (st->codecpar->width == 1920 && st->codecpar->height == 1080)) >>>>> + pkt->aspectRatio = ASPECT_16x9; >>>>> + else >>>>> + pkt->aspectRatio = ASPECT_4x3; >>>> >>>> I most likely won't use this (and I have never seen a decklink card) >>>> so please feel free to ignore: >>>> Similar code has caused some trouble with mxf files, is there >>>> really no saner solution? Like comparing what the actual aspect >>>> ratio is more similar to? Is SAR really always 1 for decklink? >>>> ("All the world's a VAX.") >>> >>> So this is definitely a confusing block of code, and you aren’t the first one to >>> ask about it (there were questions in the last round of review as well). The >>> aspect ratio referred to here is actually of the original coded video - not how >>> it’s supposed to be displayed. Hence, for example, 720x480 in widescreen >>> with a non-square PAR would still have the aspect ratio set to 4x3, since >>> that particular field describes the coded video (i.e. *not* how it’s supposed >>> to be rendered). >> >> And a resolution of 1024x576 does not exist for decklink? >> >> What about 1920x1088? >> > > The Blackmagic cards have a fairly constrained set of modes which are supported (see the definition of BMDDisplayMode in the Decklink SDK documentation). Neither of the modes you mentioned are available for output, although I’m not sure what st->codecpar->width would be set to if swscale is used between the decoder and the output. > > Bear in mind, I’m not particularly happy with how that block of code is done either (which is why I marked it with a FIXME), but it’s probably good enough for 95% of use cases. > > Devin Technically, there are a number of 2K and 4K video modes supported by some DeckLink cards that have a 16x9 aspect ratio as well. This code would treat such video modes at 4:3. The various 4K DCI video modes supported by Blackmagic use an aspect ratio of 256:135, although perhaps it is sufficient to treat those as 16:9. Perhaps the logic should be, anything with a width of 1280 or greater (or a height of 720 or greater) is 16:9--everything else can be treated as 4:3. Admittedly, libklvanc may not support VANC for 2K and 4K video anyway, but the Blackmagic devices do support it, so if you want to rule those out, then some explicit code to only support up to 720p/1080i (maybe 1080p?) probably ought to be added. On a separate note, based on a similar review I did awhile back, if this set of patches only supports a specific set of video modes, then there probably ought to be checks to make sure the code changes are only exercised for those video modes. Aaron Levinson
Hello Aaron, Thanks for the feedback. Comments inline. On Sat, Dec 30, 2017 at 12:34 AM, Aaron Levinson <alevinsn_dev@levland.net> wrote: > Technically, there are a number of 2K and 4K video modes supported by some > DeckLink cards that have a 16x9 aspect ratio as well. This code would treat > such video modes at 4:3. The various 4K DCI video modes supported by > Blackmagic use an aspect ratio of 256:135, although perhaps it is sufficient > to treat those as 16:9. Perhaps the logic should be, anything with a width > of 1280 or greater (or a height of 720 or greater) is 16:9--everything else > can be treated as 4:3. While SMPTE 2016-1 doesn't support anything above 1080p, I'm not against a bit of future-proofing, in particular given this code is already pretty ugly. My thought would be to do (width / height) and if the value is (<= 1.33) then treat it as 4x3. Otherwise treat it as 16:9. That seems general enough, and will ensure that any higher resolutions which get introduced won't require a code change. > Admittedly, libklvanc may not support VANC for 2K and 4K video anyway, but > the Blackmagic devices do support it, so if you want to rule those out, then > some explicit code to only support up to 720p/1080i (maybe 1080p?) probably > ought to be added. I'm not against such a change, given I haven't done any 4K testing. Once some validation has been done then we can look to pull the check out. > On a separate note, based on a similar review I did awhile back, if this set > of patches only supports a specific set of video modes, then there probably > ought to be checks to make sure the code changes are only exercised for > those video modes. The goal at this point is to support all the video modes typically seen in the broadcast industry. We've got a flag called "ctx->supports_vanc" which drives whether any of the VANC processing happens, and I guess we could have some sort of list of supported formats and the rest would have it disabled. Then we an add the 4K and RGB modes once they get some attention. Devin
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 0972df0bde..4f7b6df09d 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -1328,6 +1328,12 @@ enum AVPacketSideDataType { AV_PKT_DATA_A53_CC, /** + * Active Format Description data consisting of a single byte as specified + * in ETSI TS 101 154 using AVActiveFormatDescription enum. + */ + AV_PKT_DATA_AFD, + + /** * The number of side data types. * This is not part of the public API/ABI in the sense that it may * change when new side data types are added. diff --git a/libavcodec/v210enc.c b/libavcodec/v210enc.c index 51804e3bac..26a08a1ec9 100644 --- a/libavcodec/v210enc.c +++ b/libavcodec/v210enc.c @@ -243,6 +243,15 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt, return AVERROR(ENOMEM); } + side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_AFD); + if (side_data && side_data->size) { + uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_AFD, side_data->size); + if (buf) + memcpy(buf, side_data->data, side_data->size); + else + return AVERROR(ENOMEM); + } + pkt->flags |= AV_PKT_FLAG_KEY; *got_packet = 1; return 0; diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp index ded47108d6..59a4181e19 100644 --- a/libavdevice/decklink_enc.cpp +++ b/libavdevice/decklink_enc.cpp @@ -284,7 +284,8 @@ av_cold int ff_decklink_write_trailer(AVFormatContext *avctx) #if CONFIG_LIBKLVANC static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx *ctx, - AVPacket *pkt, decklink_frame *frame) + AVPacket *pkt, decklink_frame *frame, + AVStream *st) { struct klvanc_line_set_s vanc_lines = { 0 }; int ret, size; @@ -342,6 +343,46 @@ static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx * } } + data = av_packet_get_side_data(pkt, AV_PKT_DATA_AFD, &size); + if (data) { + struct klvanc_packet_afd_s *pkt; + uint16_t *afd; + uint16_t len; + + ret = klvanc_create_AFD(&pkt); + if (ret != 0) + return AVERROR(ENOMEM); + + ret = klvanc_set_AFD_val(pkt, data[0]); + if (ret != 0) { + av_log(avctx, AV_LOG_ERROR, "Invalid AFD value specified: %d\n", + data[0]); + klvanc_destroy_AFD(pkt); + return AVERROR(EINVAL); + } + + /* FIXME: Should really rely on the coded_width but seems like that + is not accessible to libavdevice outputs */ + if ((st->codecpar->width == 1280 && st->codecpar->height == 720) || + (st->codecpar->width == 1920 && st->codecpar->height == 1080)) + pkt->aspectRatio = ASPECT_16x9; + else + pkt->aspectRatio = ASPECT_4x3; + + ret = klvanc_convert_AFD_to_words(pkt, &afd, &len); + if (ret != 0) { + av_log(avctx, AV_LOG_ERROR, "Failed converting 708 packet to words\n"); + return AVERROR(ENOMEM); + } + klvanc_destroy_AFD(pkt); + + ret = klvanc_line_insert(ctx->vanc_ctx, &vanc_lines, afd, len, 12, 0); + if (ret != 0) { + av_log(avctx, AV_LOG_ERROR, "VANC line insertion failed\n"); + return AVERROR(ENOMEM); + } + } + IDeckLinkVideoFrameAncillary *vanc; int result = ctx->dlo->CreateAncillaryData(bmdFormat10BitYUV, &vanc); if (result != S_OK) { @@ -437,7 +478,7 @@ static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt) frame = new decklink_frame(ctx, avpacket, st->codecpar->codec_id, ctx->bmd_height, ctx->bmd_width); #if CONFIG_LIBKLVANC - ret = decklink_construct_vanc(avctx, ctx, pkt, frame); + ret = decklink_construct_vanc(avctx, ctx, pkt, frame, st); if (ret != 0) { av_log(avctx, AV_LOG_ERROR, "Failed to construct VANC\n"); }
Implement support for including AFD in decklink output. This includes making sure the AFD data is preserved when going from an AVFrame to a V210 packet (needed for 10-bit support). Updated to reflect feedback from Marton Balint <cus@passwd.hu> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> --- libavcodec/avcodec.h | 6 ++++++ libavcodec/v210enc.c | 9 +++++++++ libavdevice/decklink_enc.cpp | 45 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 58 insertions(+), 2 deletions(-)