Re: [FFmpeg-devel] [PATCH] tcp: properly return EOF
On 12/30/2017 8:44 AM, wm4 wrote: There is no POSIX error code for EOF - recv() signals EOF by simply returning 0. But libavformat recently changed its conventionts and "conventionts" -> "conventions" requires an explicit AVERROR_EOF, or it might get into an endless retry loop, consuming 100% CPU while doing nothing. --- libavformat/tcp.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavformat/tcp.c b/libavformat/tcp.c index fef0729da6..8773493df1 100644 --- a/libavformat/tcp.c +++ b/libavformat/tcp.c @@ -225,6 +225,8 @@ static int tcp_read(URLContext *h, uint8_t *buf, int size) return ret; } ret = recv(s->fd, buf, size, 0); +if (ret == 0) +return AVERROR_EOF; return ret < 0 ? ff_neterrno() : ret; } LGTM ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 8/8] decklink: Add support for compressed AC-3 output over SDI
* Length code */ +s337_payload[7] = bitcount >> 8; /* Length code */ +s337_payload_start = _payload[8]; +for (i = 0; i < pkt->size; i += 2) { +s337_payload_start[0] = pkt->data[i+1]; +s337_payload_start[1] = pkt->data[i]; +s337_payload_start += 2; +} + +return 0; +} + static int decklink_write_audio_packet(AVFormatContext *avctx, AVPacket *pkt) { struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data; struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx; -int sample_count = pkt->size / (ctx->channels << 1); +AVStream *st = avctx->streams[pkt->stream_index]; +int sample_count; buffercount_type buffered; +uint8_t *outbuf = NULL; +int ret = 0; ctx->dlo->GetBufferedAudioSampleFrameCount(); if (pkt->pts > 1 && !buffered) av_log(avctx, AV_LOG_WARNING, "There's no buffered audio." " Audio will misbehave!\n"); -if (ctx->dlo->ScheduleAudioSamples(pkt->data, sample_count, pkt->pts, +if (st->codecpar->codec_id == AV_CODEC_ID_AC3) { +/* Encapsulate AC3 syncframe into SMPTE 337 packet */ +ret = create_s337_payload(pkt, st->codecpar->codec_id, + , _count); +if (ret != 0) Assuming that you make the change discussed above to create_s337_payload(), can change to return ret here in case of failure. +goto done; +} else { +sample_count = pkt->size / (ctx->channels << 1); +outbuf = pkt->data; +} + +if (ctx->dlo->ScheduleAudioSamples(outbuf, sample_count, pkt->pts, bmdAudioSampleRate48kHz, NULL) != S_OK) { Simple approach that eliminates the need for using goto and the done label: store the return value of ScheduleAudioSamples() in a local variable. Then, free outbuf if appropriate. Then handle the ScheduleAudioSamples() failure case. Eliminating the goto will also make the code easier to understand, and it will result in the memory being deallocated immediately after it is no longer needed. av_log(avctx, AV_LOG_ERROR, "Could not schedule audio samples.\n"); -return AVERROR(EIO); +ret = AVERROR(EIO); +goto done; } -return 0; +done: +if (st->codecpar->codec_id == AV_CODEC_ID_AC3) +av_free(outbuf); + +return ret; } extern "C" { Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 7/8] decklink: Add support for SCTE-104 to decklink capture
ack_context, struct klvanc_context_s *ctx, + struct klvanc_packet_scte_104_s *pkt) +{ +struct vanc_cb_ctx *cb_ctx = (struct vanc_cb_ctx *)callback_context; +decklink_cctx *decklink_cctx = (struct decklink_cctx *)cb_ctx->avctx->priv_data; +struct decklink_ctx *decklink_ctx = (struct decklink_ctx *)decklink_cctx->ctx; +AVPacket avpkt; +av_init_packet(); + +avpkt.stream_index = -1; +for (int i = 0; i < decklink_ctx->num_data_streams; i++) { +if (decklink_ctx->data_st[i]->codecpar->codec_id = AV_CODEC_ID_SCTE_104) { +avpkt.stream_index = decklink_ctx->data_st[i]->index; +break; +} +} +if (avpkt.stream_index == -1) { +/* SCTE-104 packet received but forwarding is disabled */ +return 0; +} + +avpkt.pts = cb_ctx->pkt->pts; +avpkt.dts = cb_ctx->pkt->dts; +avpkt.data = pkt->payload; +avpkt.size = pkt->payloadLengthBytes; +if (avpacket_queue_put(_ctx->queue, ) < 0) { +++decklink_ctx->dropped; +} + +return 0; +} + static struct klvanc_callbacks_s callbacks = { cb_AFD, cb_EIA_708B, NULL, -NULL, +cb_SCTE_104, NULL, NULL, }; @@ -1285,6 +1341,9 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) ctx->teletext_st = st; } +/* Setup streams. */ +setup_data(avctx); + if (cctx->audio_mode == AUDIO_MODE_DISCRETE) { av_log(avctx, AV_LOG_VERBOSE, "Using %d input audio channels\n", ctx->audio_st[0]->codecpar->channels); result = ctx->dli->EnableAudioInput(bmdAudioSampleRate48kHz, diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c index d3d8c848cf..122da08fa1 100644 --- a/libavdevice/decklink_dec_c.c +++ b/libavdevice/decklink_dec_c.c @@ -76,6 +76,7 @@ static const AVOption options[] = { { "draw_bars", "draw bars on signal loss" , OFFSET(draw_bars), AV_OPT_TYPE_BOOL, { .i64 = 1}, 0, 1, DEC }, { "queue_size","input queue buffer size", OFFSET(queue_size), AV_OPT_TYPE_INT64, { .i64 = (1024 * 1024 * 1024)}, 0, INT64_MAX, DEC }, { "audio_depth", "audio bitdepth (16 or 32)", OFFSET(audio_depth), AV_OPT_TYPE_INT, { .i64 = 16}, 16, 32, DEC }, +{ "enable_scte_104", "capture SCTE-104 VANC", OFFSET(enable_scte_104), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, DEC, "enable_scte_104"}, { NULL }, }; diff --git a/libavdevice/version.h b/libavdevice/version.h index 364404d65e..0d4477f82e 100644 --- a/libavdevice/version.h +++ b/libavdevice/version.h @@ -29,7 +29,7 @@ #define LIBAVDEVICE_VERSION_MAJOR 58 #define LIBAVDEVICE_VERSION_MINOR 0 -#define LIBAVDEVICE_VERSION_MICRO 100 +#define LIBAVDEVICE_VERSION_MICRO 101 #define LIBAVDEVICE_VERSION_INT AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \ LIBAVDEVICE_VERSION_MINOR, \ Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 6/8] Add suppoort for using libklvanc from within decklink capture module
InputFrameArrived( IDeckLinkVideoInputFrame *videoFrame, IDeckLinkAudioInputPacket *audioFrame) { decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data; +struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx; void *frameBytes; void *audioFrameBytes; BMDTimeValue frameTime; @@ -785,10 +899,17 @@ HRESULT decklink_input_callback::VideoInputFrameArrived( for (i = vanc_line_numbers[idx].vanc_start; i <= vanc_line_numbers[idx].vanc_end; i++) { uint8_t *buf; if (vanc->GetBufferForVerticalBlankingLine(i, (void**)) == S_OK) { +#if CONFIG_LIBKLVANC +int ret = klvanc_handle_line(avctx, ctx->vanc_ctx, + buf, videoFrame->GetWidth(), i, ); +if (ret != 0) +av_log(avctx, AV_LOG_ERROR, "Error parsing VANC for line %d\n", i); +#else uint16_t luma_vanc[MAX_WIDTH_VANC]; extract_luma_from_v210(luma_vanc, buf, videoFrame->GetWidth()); txt_buf = get_metadata(avctx, luma_vanc, videoFrame->GetWidth(), txt_buf, sizeof(txt_buf0) - (txt_buf - txt_buf0), ); +#endif } if (i == vanc_line_numbers[idx].field0_vanc_end) i = vanc_line_numbers[idx].field1_vanc_start - 1; @@ -950,6 +1071,7 @@ av_cold int ff_decklink_read_close(AVFormatContext *avctx) ff_decklink_cleanup(avctx); avpacket_queue_end(>queue); +klvanc_context_destroy(ctx->vanc_ctx); Shouldn't this last line be wrapped in #if CONFIG_LIBKLVANC? I suggest building this on Linux with and without libklvanc, and I also suggest building it (and ideally testing it) on Windows without libklvanc as well. DeckLink is also supported on OS/X. av_freep(>ctx); @@ -1193,6 +1315,17 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) avpacket_queue_init (avctx, >queue); +#if CONFIG_LIBKLVANC +if (klvanc_context_create(>vanc_ctx) < 0) { +av_log(avctx, AV_LOG_ERROR, "Cannot create VANC library context\n"); +ret = AVERROR(ENOMEM); Perhaps appropriate to use the return value of klvanc_context_create() as input to AVERROR(). +goto error; +} else { +ctx->vanc_ctx->verbose = 0; +ctx->vanc_ctx->callbacks = +} +#endif + if (ctx->dli->StartStreams() != S_OK) { av_log(avctx, AV_LOG_ERROR, "Cannot start input stream\n"); ret = AVERROR(EIO); Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 5/8] Support encoding of Active Format Description (AFD) in libx264
dp), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, {"a53cc", "Use A53 Closed Captions (if available)", OFFSET(a53_cc),AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, VE}, +{"afd","Use Active Format Description (AFD) (if available)",OFFSET(afd),AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, VE}, {"x264opts", "x264 options", OFFSET(x264opts), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, { "crf", "Select the quality for constant quality mode", OFFSET(crf), AV_OPT_TYPE_FLOAT, {.dbl = -1 }, -1, FLT_MAX, VE }, { "crf_max", "In CRF mode, prevents VBV from lowering quality beyond this point.",OFFSET(crf_max), AV_OPT_TYPE_FLOAT, {.dbl = -1 }, -1, FLT_MAX, VE }, diff --git a/libavcodec/utils.c b/libavcodec/utils.c index baf09119fe..5fc925b8cd 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2403,6 +2403,42 @@ int ff_alloc_a53_sei(const AVFrame *frame, size_t prefix_len, return 0; } +/* Defined in SCTE 128-1 2013 Sec 8.1 */ +int ff_alloc_afd_sei(const AVFrame *frame, size_t prefix_len, + void **data, size_t *sei_size) +{ +AVFrameSideData *side_data = NULL; +uint8_t *sei_data; + +if (frame) +side_data = av_frame_get_side_data(frame, AV_FRAME_DATA_AFD); + +if (!side_data) { +*data = NULL; +return 0; +} + +*sei_size = 9; +*data = av_mallocz(*sei_size + prefix_len); +if (!*data) +return AVERROR(ENOMEM); +sei_data = (uint8_t*)*data + prefix_len; + +/* country code (SCTE 128-1 Sec 8.1.1) */ +sei_data[0] = 181; +sei_data[1] = 0; +sei_data[2] = 49; + +/* country code (SCTE 128-1 Sec 8.1.2) */ +AV_WL32(sei_data + 3, MKTAG('D', 'T', 'G', '1')); + +/* country code (SCTE 128-1 Sec 8.2.5) */ +sei_data[7] = 0x41; +sei_data[8] = 0xf0 | side_data->data[0]; + +return 0; +} + int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) { AVRational framerate = avctx->framerate; I don't know enough about this to review the technical aspects of this code, but I will say that these changes don't help those users that are encoding in H.264 but using hardware encoders such as Intel QuickSync. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/8] decklink: Introduce support for capture of multiple audio streams
); +audio_offset += sample_size; +} } } @@ -999,18 +1077,7 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) #endif /* Setup streams. */ -st = avformat_new_stream(avctx, NULL); -if (!st) { -av_log(avctx, AV_LOG_ERROR, "Cannot add stream\n"); -ret = AVERROR(ENOMEM); -goto error; -} -st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; -st->codecpar->codec_id= cctx->audio_depth == 32 ? AV_CODEC_ID_PCM_S32LE : AV_CODEC_ID_PCM_S16LE; -st->codecpar->sample_rate = bmdAudioSampleRate48kHz; -st->codecpar->channels= cctx->audio_channels; -avpriv_set_pts_info(st, 64, 1, 100); /* 64 bits pts in us */ -ctx->audio_st=st; +setup_audio(avctx); st = avformat_new_stream(avctx, NULL); if (!st) { @@ -1096,8 +1163,17 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) ctx->teletext_st = st; } -av_log(avctx, AV_LOG_VERBOSE, "Using %d input audio channels\n", ctx->audio_st->codecpar->channels); -result = ctx->dli->EnableAudioInput(bmdAudioSampleRate48kHz, cctx->audio_depth == 32 ? bmdAudioSampleType32bitInteger : bmdAudioSampleType16bitInteger, ctx->audio_st->codecpar->channels); +if (cctx->audio_mode == AUDIO_MODE_DISCRETE) { +av_log(avctx, AV_LOG_VERBOSE, "Using %d input audio channels\n", ctx->audio_st[0]->codecpar->channels); +result = ctx->dli->EnableAudioInput(bmdAudioSampleRate48kHz, +ctx->audio_depth == 32 ? bmdAudioSampleType32bitInteger : bmdAudioSampleType16bitInteger, + ctx->audio_st[0]->codecpar->channels); +} else { +av_log(avctx, AV_LOG_VERBOSE, "Using %d input audio channels\n", (int)ctx->max_audio_channels); +result = ctx->dli->EnableAudioInput(bmdAudioSampleRate48kHz, +ctx->audio_depth == 32 ? bmdAudioSampleType32bitInteger : bmdAudioSampleType16bitInteger, +ctx->max_audio_channels); +} if (result != S_OK) { av_log(avctx, AV_LOG_ERROR, "Cannot enable audio input\n"); diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c index 1c6d826945..d3d8c848cf 100644 --- a/libavdevice/decklink_dec_c.c +++ b/libavdevice/decklink_dec_c.c @@ -44,6 +44,9 @@ static const AVOption options[] = { { "standard", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0x7fff9fffeLL}, 0, 0,DEC, "teletext_lines"}, { "all", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0x7LL}, 0, 0,DEC, "teletext_lines"}, { "channels", "number of audio channels", OFFSET(audio_channels), AV_OPT_TYPE_INT , { .i64 = 2 }, 2, 16, DEC }, +{ "audio_mode", "audio mode", OFFSET(audio_mode), AV_OPT_TYPE_INT, { .i64 = AUDIO_MODE_DISCRETE}, 0, 1,DEC, "audio_mode"}, +{ "discrete", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AUDIO_MODE_DISCRETE}, 0, 0,DEC, "audio_mode"}, +{ "pairs",NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AUDIO_MODE_PAIRS}, 0, 0,DEC, "audio_mode"}, { "duplex_mode", "duplex mode", OFFSET(duplex_mode), AV_OPT_TYPE_INT, { .i64 = 0}, 0, 2,DEC, "duplex_mode"}, { "unset", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0}, 0, 0,DEC, "duplex_mode"}, { "half", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1}, 0, 0,DEC, "duplex_mode"}, Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI
On 12/29/2017 10:12 AM, Devin Heitmueller wrote: Hook in libklvanc and use it for output of EIA-708 captions over SDI. The bulk of this patch is just general support for ancillary data for the Decklink SDI module - the real work for construction of the EIA-708 CDP and VANC line construction is done by libklvanc. Libklvanc can be found at: https://github.com/stoth68000/libklvanc Updated to reflect feedback from Marton BalintSigned-off-by: Devin Heitmueller --- configure | 4 ++ libavcodec/v210enc.c| 10 +++ libavdevice/decklink_common.cpp | 17 +++-- libavdevice/decklink_common.h | 10 +++ libavdevice/decklink_enc.cpp| 154 ++-- 5 files changed, 185 insertions(+), 10 deletions(-) diff --git a/configure b/configure index 6748ef8bc9..19fd04bb6f 100755 --- a/configure +++ b/configure @@ -238,6 +238,7 @@ External library support: --enable-libiec61883 enable iec61883 via libiec61883 [no] --enable-libilbc enable iLBC de/encoding via libilbc [no] --enable-libjack enable JACK audio sound server [no] + --enable-libklvanc enable Kernel Labs VANC processing [no] --enable-libkvazaar enable HEVC encoding via libkvazaar [no] --enable-libmodplug enable ModPlug via libmodplug [no] --enable-libmp3lame enable MP3 encoding via libmp3lame [no] @@ -1602,6 +1603,7 @@ EXTERNAL_LIBRARY_LIST=" libiec61883 libilbc libjack +libklvanc libkvazaar libmodplug libmp3lame @@ -3074,6 +3076,7 @@ decklink_deps_any="libdl LoadLibrary" decklink_indev_deps="decklink threads" decklink_indev_extralibs="-lstdc++" decklink_outdev_deps="decklink threads" +decklink_outdev_suggest="libklvanc" decklink_outdev_extralibs="-lstdc++" libndi_newtek_indev_deps="libndi_newtek" libndi_newtek_indev_extralibs="-lndi" @@ -5835,6 +5838,7 @@ enabled libgsm&& { for gsm_hdr in "gsm.h" "gsm/gsm.h"; do check_lib libgsm "${gsm_hdr}" gsm_create -lgsm && break; done || die "ERROR: libgsm not found"; } enabled libilbc && require libilbc ilbc.h WebRtcIlbcfix_InitDecode -lilbc $pthreads_extralibs +enabled libklvanc && require libklvanc libklvanc/vanc.h klvanc_context_create -lklvanc enabled libkvazaar&& require_pkg_config libkvazaar "kvazaar >= 0.8.1" kvazaar.h kvz_api_get # While it may appear that require is being used as a pkg-config # fallback for libmfx, it is actually being used to detect a different diff --git a/libavcodec/v210enc.c b/libavcodec/v210enc.c index a6afbbfc41..51804e3bac 100644 --- a/libavcodec/v210enc.c +++ b/libavcodec/v210enc.c @@ -123,6 +123,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt, int aligned_width = ((avctx->width + 47) / 48) * 48; int stride = aligned_width * 8 / 3; int line_padding = stride - ((avctx->width * 8 + 11) / 12) * 4; +AVFrameSideData *side_data; int h, w, ret; uint8_t *dst; @@ -233,6 +234,15 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt, } } +side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_A53_CC); +if (side_data && side_data->size) { +uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, 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_common.cpp b/libavdevice/decklink_common.cpp index 6ef2c529f4..ba091dadea 100644 --- a/libavdevice/decklink_common.cpp +++ b/libavdevice/decklink_common.cpp @@ -254,10 +254,19 @@ int ff_decklink_set_format(AVFormatContext *avctx, , NULL) != S_OK) return -1; } else { -if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, bmdFormat8BitYUV, - bmdVideoOutputFlagDefault, - , NULL) != S_OK) -return -1; +ctx->supports_vanc = 1; +if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, ctx->raw_format, + bmdVideoOutputVANC, + , NULL) != S_OK) { +/* Try again, but without VANC enabled */ +if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, ctx->raw_format, + bmdVideoOutputFlagDefault, + , NULL) != S_OK) { +return -1; +} +ctx->supports_vanc = 0; +} + } if (support == bmdDisplayModeSupported) return 0; diff --git a/libavdevice/decklink_common.h
Re: [FFmpeg-devel] [PATCH 2/8] decklink: Add support for output of Active Format Description (AFD)
On 12/29/2017 1:20 PM, Devin Heitmueller wrote: On Dec 29, 2017, at 4:09 PM, Carl Eugen Hoyos <ceffm...@gmail.com> wrote: 2017-12-29 22:02 GMT+01:00 Devin Heitmueller <dheitmuel...@ltnglobal.com>: On Dec 29, 2017, at 3:48 PM, Carl Eugen Hoyos <ceffm...@gmail.com> wrote: 2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmuel...@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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [MSVC toolchain] Patch to allow building FFmpeg with Linux bash on Windows (WSL)
On 12/28/2017 6:43 PM, Cyber Sinh wrote: The attached patch changes the configure script for FFmpeg (and associated shell scripts) to call MSVC tools including their extensions (cl.exe instead of cl for example). This is necessary, because WSL can automatically launch Windows processes from the Linux side but only if the process is in the path and includes the full name. Linux doesn't automatically append .exe and such to invoke a file. I haven't confirmed yet that this works in the msys2 environment on Windows, although according to https://blogs.msdn.microsoft.com/gillesk/2016/12/02/building-ffmpeg-using-wsl/ , it supposedly shouldn't cause any issues with building under msys2 or WSL. It appears all of the contents of this patch have been taken from another patch, available at https://github.com/Microsoft/FFmpegInterop/blob/gillesk/wsl/0001-Updating-scripts-to-run-under-WSL.patch , although its reasonable that the author of this patch would have come up with it on their own since it is so simple. It would be reasonable to give credit to Gilles Khouzam in the patch description, if the author of this patch (Cyber Sinh) and Gilles Khouzam are not the same individual. Also, Gilles's patch does more to get FFmpeg to build properly under WSL, so just adding the file extension may not be sufficient to get FFmpeg to build under WSL. On a separate note, building under WSL, as opposed to msys2, seems promising if only from a build performance perspective. According to the blog post, it builds at least twice as fast under WSL than it does with msys2. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] fix MSVC compilation errors
On 12/9/2017 11:25 PM, Rostislav Pehlivanov wrote: On 10 December 2017 at 02:41, Aaron Levinson <alevinsn_...@levland.net> wrote: On 12/9/2017 6:24 PM, Aaron Levinson wrote: On 12/9/2017 6:15 PM, Aaron Levinson wrote: On 12/9/2017 1:18 AM, Hendrik Leppkes wrote: On Fri, Dec 8, 2017 at 8:49 AM, Mateusz <mateu...@poczta.onet.pl> wrote: W dniu 07.12.2017 o 22:58, Hendrik Leppkes pisze: Am 07.12.2017 20:40 schrieb "Mateusz" <mateu...@poczta.onet.pl>: W dniu 07.12.2017 o 10:42, Hendrik Leppkes pisze: On Thu, Dec 7, 2017 at 2:02 AM, Mateusz <mateu...@poczta.onet.pl> wrote: After commit 3701d49 'error_resilience: remove avpriv_atomic usage' we have included windows.h in much more files and we should avoid conflicts with defines/function declarations. We should declare compatible variables for atomic compat wrappers that expect fixed size variables in atomic_compare_exchange* macro. Signed-off-by: Mateusz Brzostek <mateu...@poczta.onet.pl> --- libavcodec/jpegls.h | 2 ++ libavcodec/mjpegdec.h | 2 ++ libavcodec/mss2.c | 6 +++--- libavcodec/utils.c| 12 libavformat/mxfenc.c | 2 +- 5 files changed, 20 insertions(+), 4 deletions(-) diff --git a/libavcodec/jpegls.h b/libavcodec/jpegls.h index c8997c7861..6b89b2afa3 100644 --- a/libavcodec/jpegls.h +++ b/libavcodec/jpegls.h @@ -32,6 +32,8 @@ #include "avcodec.h" #include "internal.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + typedef struct JpeglsContext { AVCodecContext *avctx; } JpeglsContext; diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h index c84a40aa6e..c36fba5f22 100644 --- a/libavcodec/mjpegdec.h +++ b/libavcodec/mjpegdec.h @@ -39,6 +39,8 @@ #include "hpeldsp.h" #include "idctdsp.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + #define MAX_COMPONENTS 4 typedef struct MJpegDecodeContext { diff --git a/libavcodec/mss2.c b/libavcodec/mss2.c index 9e7cc466de..3180af1d60 100644 --- a/libavcodec/mss2.c +++ b/libavcodec/mss2.c @@ -464,9 +464,9 @@ static int decode_wmv9(AVCodecContext *avctx, const uint8_t *buf, int buf_size, return 0; } -typedef struct Rectangle { +struct Rectangle { int coded, x, y, w, h; -} Rectangle; +}; #define MAX_WMV9_RECTANGLES 20 #define ARITH2_PADDING 2 @@ -485,7 +485,7 @@ static int mss2_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, int keyframe, has_wmv9, has_mv, is_rle, is_555, ret; -Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; +struct Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; int used_rects = 0, i, implicit_rect = 0, av_uninit(wmv9_mask); if ((ret = init_get_bits8(, buf, buf_size)) < 0) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index baf09119fe..70a0764714 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1943,7 +1943,13 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op)) int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ +!defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 0; +#else +atomic_bool exp = 0; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; @@ -1969,7 +1975,13 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) int ff_unlock_avcodec(const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ +!defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 1; +#else +atomic_bool exp = 1; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; These ifdefs here are very ugly, and as mentioned in another mail, the atomics in those two functions arent even required - all access to those variables is supposed to be protected by the lockmgr anyway. So it would be easier to just remove any atomic nature of those variables (or at the very lease replace the compare_exchange with a store to solve this problem at hand). I'm not sure but are you proposed to revert commit https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99 ab5432543d 47a559a461 Basically, yes. Atomics are not needed for this variable, as access to it should be serialized anyways. OK for me. I've sent smaller patch (only near/Rectangle stuff). LGTM. This patch doesn't apply anymore to the latest code base due to changes to mxfenc.c. I should add that the
Re: [FFmpeg-devel] [PATCH] lavc/utils: remove unnecessary locking
On 12/8/2017 2:27 AM, Michael Niedermayer wrote: On Fri, Dec 08, 2017 at 09:49:25AM +0100, Hendrik Leppkes wrote: On Fri, Dec 8, 2017 at 6:09 AM, Rostislav Pehlivanov <atomnu...@gmail.com> wrote: Its already done by lockmgr. Signed-off-by: Rostislav Pehlivanov <atomnu...@gmail.com> --- libavcodec/utils.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index baf09119fe..796d24dcbb 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -115,7 +115,6 @@ static int (*lockmgr_cb)(void **mutex, enum AVLockOp op) = NULL; #endif -static atomic_bool ff_avcodec_locked; static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0); static void *codec_mutex; static void *avformat_mutex; @@ -1943,7 +1942,6 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op)) int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) { -_Bool exp = 0; if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; @@ -1959,21 +1957,17 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) atomic_load(_thread_counter)); if (!lockmgr_cb) av_log(log_ctx, AV_LOG_ERROR, "No lock manager is set, please see av_lockmgr_register()\n"); -atomic_store(_avcodec_locked, 1); ff_unlock_avcodec(codec); return AVERROR(EINVAL); } -av_assert0(atomic_compare_exchange_strong(_avcodec_locked, , 1)); return 0; } int ff_unlock_avcodec(const AVCodec *codec) { -_Bool exp = 1; if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; -av_assert0(atomic_compare_exchange_strong(_avcodec_locked, , 0)); atomic_fetch_add(_thread_counter, -1); if (lockmgr_cb) { if ((*lockmgr_cb)(_mutex, AV_LOCK_RELEASE)) -- 2.15.1.424.g9478a66081 These variables never performed any locking, they only existed as a sanity check that lock/unlock is always called in pairs. If that is really required is up for discussion. They shuld be usefull to detect some bugs related to locking [...] I don't have the most recent response to this e-mail, from Hendrik, in my inbox anymore, but I spent some time reviewing the patch, and I also don't see any point to making access to ff_avcodec_locked atomic. Prior to patch https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99ab5432543d47a559a461 , ff_avcodec_locked was declared as volatile and also specified as an extern in internal.h. Neither the volatile declaration nor the global exposure of ff_avcodec_locked are necessary. The patch eliminated the global exposure, but it replaced "volatile" with "atomic". Write access to ff_avcodec_locked is already protected via lockmgr_cb. If, somehow, lockmgr_cb is NULL, which shouldn't happen in practice, the entangled_thread_counter backup approach that immediately follows isn't sufficient as currently implemented to protect writes to ff_avcodec_locked, which makes me wonder why it is there in the first place. It is possible to use entangled_thread_counter in a way that protects access to ff_avcodec_locked though, and I think that's the intention of the code. I think the correct approach is to mostly restore the code prior to patch https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99ab5432543d47a559a461 but to leave ff_avcodec_locked statically declared and eliminate the volatile designation. On a separate note, this whole approach of using ff_lock_avcodec/ff_unlock_avcodec seems a little hokey to me. It is effectively using a global lock (via lockmgr_cb) to control access to avctx, which should be local to the calling thread. As implemented, it prevents two concurrent calls to avcodec_open2() from proceeding simultaneously. As far as I can tell, the implementation of avcodec_open2() doesn't modify any global structures and only modifies avctx. I would think that a better approach would be to use a lock that is specific to the avctx object, perhaps one stored in avctx->internal. This approach would also eliminate the codec_mutex memory leak. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] fix MSVC compilation errors
On 12/9/2017 6:24 PM, Aaron Levinson wrote: On 12/9/2017 6:15 PM, Aaron Levinson wrote: On 12/9/2017 1:18 AM, Hendrik Leppkes wrote: On Fri, Dec 8, 2017 at 8:49 AM, Mateusz <mateu...@poczta.onet.pl> wrote: W dniu 07.12.2017 o 22:58, Hendrik Leppkes pisze: Am 07.12.2017 20:40 schrieb "Mateusz" <mateu...@poczta.onet.pl>: W dniu 07.12.2017 o 10:42, Hendrik Leppkes pisze: On Thu, Dec 7, 2017 at 2:02 AM, Mateusz <mateu...@poczta.onet.pl> wrote: After commit 3701d49 'error_resilience: remove avpriv_atomic usage' we have included windows.h in much more files and we should avoid conflicts with defines/function declarations. We should declare compatible variables for atomic compat wrappers that expect fixed size variables in atomic_compare_exchange* macro. Signed-off-by: Mateusz Brzostek <mateu...@poczta.onet.pl> --- libavcodec/jpegls.h | 2 ++ libavcodec/mjpegdec.h | 2 ++ libavcodec/mss2.c | 6 +++--- libavcodec/utils.c | 12 libavformat/mxfenc.c | 2 +- 5 files changed, 20 insertions(+), 4 deletions(-) diff --git a/libavcodec/jpegls.h b/libavcodec/jpegls.h index c8997c7861..6b89b2afa3 100644 --- a/libavcodec/jpegls.h +++ b/libavcodec/jpegls.h @@ -32,6 +32,8 @@ #include "avcodec.h" #include "internal.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + typedef struct JpeglsContext { AVCodecContext *avctx; } JpeglsContext; diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h index c84a40aa6e..c36fba5f22 100644 --- a/libavcodec/mjpegdec.h +++ b/libavcodec/mjpegdec.h @@ -39,6 +39,8 @@ #include "hpeldsp.h" #include "idctdsp.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + #define MAX_COMPONENTS 4 typedef struct MJpegDecodeContext { diff --git a/libavcodec/mss2.c b/libavcodec/mss2.c index 9e7cc466de..3180af1d60 100644 --- a/libavcodec/mss2.c +++ b/libavcodec/mss2.c @@ -464,9 +464,9 @@ static int decode_wmv9(AVCodecContext *avctx, const uint8_t *buf, int buf_size, return 0; } -typedef struct Rectangle { +struct Rectangle { int coded, x, y, w, h; -} Rectangle; +}; #define MAX_WMV9_RECTANGLES 20 #define ARITH2_PADDING 2 @@ -485,7 +485,7 @@ static int mss2_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, int keyframe, has_wmv9, has_mv, is_rle, is_555, ret; - Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; + struct Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; int used_rects = 0, i, implicit_rect = 0, av_uninit(wmv9_mask); if ((ret = init_get_bits8(, buf, buf_size)) < 0) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index baf09119fe..70a0764714 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1943,7 +1943,13 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op)) int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ + !defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 0; +#else + atomic_bool exp = 0; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; @@ -1969,7 +1975,13 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) int ff_unlock_avcodec(const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ + !defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 1; +#else + atomic_bool exp = 1; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; These ifdefs here are very ugly, and as mentioned in another mail, the atomics in those two functions arent even required - all access to those variables is supposed to be protected by the lockmgr anyway. So it would be easier to just remove any atomic nature of those variables (or at the very lease replace the compare_exchange with a store to solve this problem at hand). I'm not sure but are you proposed to revert commit https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99ab5432543d 47a559a461 Basically, yes. Atomics are not needed for this variable, as access to it should be serialized anyways. OK for me. I've sent smaller patch (only near/Rectangle stuff). LGTM. This patch doesn't apply anymore to the latest code base due to changes to mxfenc.c. I should add that the changes to mxfenc.c aren't needed any longer. With the subpatch to mxfenc.c removed, the remain patch applies cleanly, and I can confirm that it fixes MSVC build issues as well. Aaron Levinson One more
Re: [FFmpeg-devel] [PATCH] fix MSVC compilation errors
On 12/9/2017 6:15 PM, Aaron Levinson wrote: On 12/9/2017 1:18 AM, Hendrik Leppkes wrote: On Fri, Dec 8, 2017 at 8:49 AM, Mateusz <mateu...@poczta.onet.pl> wrote: W dniu 07.12.2017 o 22:58, Hendrik Leppkes pisze: Am 07.12.2017 20:40 schrieb "Mateusz" <mateu...@poczta.onet.pl>: W dniu 07.12.2017 o 10:42, Hendrik Leppkes pisze: On Thu, Dec 7, 2017 at 2:02 AM, Mateusz <mateu...@poczta.onet.pl> wrote: After commit 3701d49 'error_resilience: remove avpriv_atomic usage' we have included windows.h in much more files and we should avoid conflicts with defines/function declarations. We should declare compatible variables for atomic compat wrappers that expect fixed size variables in atomic_compare_exchange* macro. Signed-off-by: Mateusz Brzostek <mateu...@poczta.onet.pl> --- libavcodec/jpegls.h | 2 ++ libavcodec/mjpegdec.h | 2 ++ libavcodec/mss2.c | 6 +++--- libavcodec/utils.c | 12 libavformat/mxfenc.c | 2 +- 5 files changed, 20 insertions(+), 4 deletions(-) diff --git a/libavcodec/jpegls.h b/libavcodec/jpegls.h index c8997c7861..6b89b2afa3 100644 --- a/libavcodec/jpegls.h +++ b/libavcodec/jpegls.h @@ -32,6 +32,8 @@ #include "avcodec.h" #include "internal.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + typedef struct JpeglsContext { AVCodecContext *avctx; } JpeglsContext; diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h index c84a40aa6e..c36fba5f22 100644 --- a/libavcodec/mjpegdec.h +++ b/libavcodec/mjpegdec.h @@ -39,6 +39,8 @@ #include "hpeldsp.h" #include "idctdsp.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + #define MAX_COMPONENTS 4 typedef struct MJpegDecodeContext { diff --git a/libavcodec/mss2.c b/libavcodec/mss2.c index 9e7cc466de..3180af1d60 100644 --- a/libavcodec/mss2.c +++ b/libavcodec/mss2.c @@ -464,9 +464,9 @@ static int decode_wmv9(AVCodecContext *avctx, const uint8_t *buf, int buf_size, return 0; } -typedef struct Rectangle { +struct Rectangle { int coded, x, y, w, h; -} Rectangle; +}; #define MAX_WMV9_RECTANGLES 20 #define ARITH2_PADDING 2 @@ -485,7 +485,7 @@ static int mss2_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, int keyframe, has_wmv9, has_mv, is_rle, is_555, ret; - Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; + struct Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; int used_rects = 0, i, implicit_rect = 0, av_uninit(wmv9_mask); if ((ret = init_get_bits8(, buf, buf_size)) < 0) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index baf09119fe..70a0764714 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1943,7 +1943,13 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op)) int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ + !defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 0; +#else + atomic_bool exp = 0; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; @@ -1969,7 +1975,13 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) int ff_unlock_avcodec(const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ + !defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 1; +#else + atomic_bool exp = 1; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; These ifdefs here are very ugly, and as mentioned in another mail, the atomics in those two functions arent even required - all access to those variables is supposed to be protected by the lockmgr anyway. So it would be easier to just remove any atomic nature of those variables (or at the very lease replace the compare_exchange with a store to solve this problem at hand). I'm not sure but are you proposed to revert commit https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99ab5432543d 47a559a461 Basically, yes. Atomics are not needed for this variable, as access to it should be serialized anyways. OK for me. I've sent smaller patch (only near/Rectangle stuff). LGTM. This patch doesn't apply anymore to the latest code base due to changes to mxfenc.c. I should add that the changes to mxfenc.c aren't needed any longer. With the subpatch to mxfenc.c removed, the remain patch applies cleanly, and I can confirm that it fixes MSVC build issues as well. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] fix MSVC compilation errors
On 12/9/2017 1:18 AM, Hendrik Leppkes wrote: On Fri, Dec 8, 2017 at 8:49 AM, Mateuszwrote: W dniu 07.12.2017 o 22:58, Hendrik Leppkes pisze: Am 07.12.2017 20:40 schrieb "Mateusz" : W dniu 07.12.2017 o 10:42, Hendrik Leppkes pisze: On Thu, Dec 7, 2017 at 2:02 AM, Mateusz wrote: After commit 3701d49 'error_resilience: remove avpriv_atomic usage' we have included windows.h in much more files and we should avoid conflicts with defines/function declarations. We should declare compatible variables for atomic compat wrappers that expect fixed size variables in atomic_compare_exchange* macro. Signed-off-by: Mateusz Brzostek --- libavcodec/jpegls.h | 2 ++ libavcodec/mjpegdec.h | 2 ++ libavcodec/mss2.c | 6 +++--- libavcodec/utils.c| 12 libavformat/mxfenc.c | 2 +- 5 files changed, 20 insertions(+), 4 deletions(-) diff --git a/libavcodec/jpegls.h b/libavcodec/jpegls.h index c8997c7861..6b89b2afa3 100644 --- a/libavcodec/jpegls.h +++ b/libavcodec/jpegls.h @@ -32,6 +32,8 @@ #include "avcodec.h" #include "internal.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + typedef struct JpeglsContext { AVCodecContext *avctx; } JpeglsContext; diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h index c84a40aa6e..c36fba5f22 100644 --- a/libavcodec/mjpegdec.h +++ b/libavcodec/mjpegdec.h @@ -39,6 +39,8 @@ #include "hpeldsp.h" #include "idctdsp.h" +#undef near /* This file uses struct member 'near' which in windows.h is defined as empty. */ + #define MAX_COMPONENTS 4 typedef struct MJpegDecodeContext { diff --git a/libavcodec/mss2.c b/libavcodec/mss2.c index 9e7cc466de..3180af1d60 100644 --- a/libavcodec/mss2.c +++ b/libavcodec/mss2.c @@ -464,9 +464,9 @@ static int decode_wmv9(AVCodecContext *avctx, const uint8_t *buf, int buf_size, return 0; } -typedef struct Rectangle { +struct Rectangle { int coded, x, y, w, h; -} Rectangle; +}; #define MAX_WMV9_RECTANGLES 20 #define ARITH2_PADDING 2 @@ -485,7 +485,7 @@ static int mss2_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, int keyframe, has_wmv9, has_mv, is_rle, is_555, ret; -Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; +struct Rectangle wmv9rects[MAX_WMV9_RECTANGLES], *r; int used_rects = 0, i, implicit_rect = 0, av_uninit(wmv9_mask); if ((ret = init_get_bits8(, buf, buf_size)) < 0) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index baf09119fe..70a0764714 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1943,7 +1943,13 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op)) int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ +!defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 0; +#else +atomic_bool exp = 0; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; @@ -1969,7 +1975,13 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) int ff_unlock_avcodec(const AVCodec *codec) { +#if !defined(COMPAT_ATOMICS_DUMMY_STDATOMIC_H) && !defined(COMPAT_ATOMICS_PTHREAD_STDATOMIC_H) && \ +!defined(COMPAT_ATOMICS_SUNCC_STDATOMIC_H) && !defined(COMPAT_ATOMICS_WIN32_STDATOMIC_H) _Bool exp = 1; +#else +atomic_bool exp = 1; +#endif + if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; These ifdefs here are very ugly, and as mentioned in another mail, the atomics in those two functions arent even required - all access to those variables is supposed to be protected by the lockmgr anyway. So it would be easier to just remove any atomic nature of those variables (or at the very lease replace the compare_exchange with a store to solve this problem at hand). I'm not sure but are you proposed to revert commit https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99ab5432543d 47a559a461 Basically, yes. Atomics are not needed for this variable, as access to it should be serialized anyways. OK for me. I've sent smaller patch (only near/Rectangle stuff). LGTM. This patch doesn't apply anymore to the latest code base due to changes to mxfenc.c. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure, fftools/Makefile: Copy .pdb files to bindir for MSVC builds for make install
Ping On 10/31/2017 11:05 AM, Aaron Levinson wrote: When ffmpeg is built using MSVC, symbols are stored separately from executables and libraries in .pdb files. However, unlike with gcc-based builds, when make install is invoked, symbols, in the form of .pdb files, which are important for debugging, are not copied to bindir. This change corrects this deficiency for MSVC builds. Files are also uninstalled appropriately when make uninstall is exercised. Note: General idea for how to properly handle the copying of PDB files associated with programs suggested by Hendrik Leppkes. Comments: -- configure: a) Leverage already existing SLIB_INSTALL_EXTRA_SHLIB facility (which is already pretty specific to Windows) to add .pdb files, in addition to .lib files, for shared libraries to bindir during make install. b) Add PROG_INSTALL_EXTRA_BIN variable for MSVC builds and also add it to the section that causes it to be added to config.mak. This is used in Makefile to copy any additional files associated with programs. For MSVC, it is used to copy the pdb files associated with any executables that are built. Note that such executables are build with _g in the file name and are later copied to a filename without _g in the file name. As such, the PDB files have _g in the file name. -- fftools/Makefile: Enhance install-progs and uninstall-progs targets to handle PROG_INSTALL_EXTRA_BIN if defined. It uses a similar procedure as already in place for SLIB_INSTALL_EXTRA_SHLIB in library.mak. Signed-off-by: Aaron Levinson <alevinsn_...@levland.net> --- configure| 4 +++- fftools/Makefile | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 2ac6fed98a..feb86b2069 100755 --- a/configure +++ b/configure @@ -5101,9 +5101,10 @@ case $target_os in SLIB_CREATE_DEF_CMD='$(SRC_PATH)/compat/windows/makedef $(SUBDIR)lib$(NAME).ver $(OBJS) > $$(@:$(SLIBSUF)=.def)' SLIB_INSTALL_NAME='$(SLIBNAME_WITH_MAJOR)' SLIB_INSTALL_LINKS= -SLIB_INSTALL_EXTRA_SHLIB='$(SLIBNAME:$(SLIBSUF)=.lib)' +SLIB_INSTALL_EXTRA_SHLIB='$(SLIBNAME:$(SLIBSUF)=.lib) $(SLIBNAME_WITH_MAJOR:$(SLIBSUF)=.pdb)' SLIB_INSTALL_EXTRA_LIB='$(SLIBNAME_WITH_MAJOR:$(SLIBSUF)=.def)' SHFLAGS='-dll -def:$$(@:$(SLIBSUF)=.def) -implib:$(SUBDIR)$(SLIBNAME:$(SLIBSUF)=.lib)' +PROG_INSTALL_EXTRA_BIN='$(AVPROGS-yes:%=%$(PROGSSUF)_g.pdb)' enabled x86_64 && objformat="win64" || objformat="win32" ranlib=: enable dos_paths @@ -7034,6 +7035,7 @@ SLIB_INSTALL_NAME=${SLIB_INSTALL_NAME} SLIB_INSTALL_LINKS=${SLIB_INSTALL_LINKS} SLIB_INSTALL_EXTRA_LIB=${SLIB_INSTALL_EXTRA_LIB} SLIB_INSTALL_EXTRA_SHLIB=${SLIB_INSTALL_EXTRA_SHLIB} +PROG_INSTALL_EXTRA_BIN=${PROG_INSTALL_EXTRA_BIN} VERSION_SCRIPT_POSTPROCESS_CMD=${VERSION_SCRIPT_POSTPROCESS_CMD} SAMPLES:=${samples:-\$(FATE_SAMPLES)} NOREDZONE_FLAGS=$noredzone_flags diff --git a/fftools/Makefile b/fftools/Makefile index c867814a71..8c121b9762 100644 --- a/fftools/Makefile +++ b/fftools/Makefile @@ -47,11 +47,13 @@ install-progs-$(CONFIG_SHARED): install-libs install-progs: install-progs-yes $(AVPROGS) $(Q)mkdir -p "$(BINDIR)" $(INSTALL) -c -m 755 $(AVPROGS) "$(BINDIR)" + $(if $(PROG_INSTALL_EXTRA_BIN), $(INSTALL) -m 644 $(PROG_INSTALL_EXTRA_BIN) "$(BINDIR)") uninstall: uninstall-progs uninstall-progs: $(RM) $(addprefix "$(BINDIR)/", $(ALLAVPROGS)) + $(if $(PROG_INSTALL_EXTRA_BIN), $(RM) $(addprefix "$(BINDIR)/", $(PROG_INSTALL_EXTRA_BIN))) clean:: $(RM) $(ALLAVPROGS) $(ALLAVPROGS_G) $(CLEANSUFFIXES:%=fftools/%) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avdevice/decklink_dec: make some function static
On 11/8/2017 7:20 PM, James Almer wrote: Should fix ticket #6822 Signed-off-by: James Almer--- libavdevice/decklink_dec.cpp | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp index 191547ff10..e90b42811a 100644 --- a/libavdevice/decklink_dec.cpp +++ b/libavdevice/decklink_dec.cpp @@ -262,8 +262,8 @@ static uint8_t* teletext_data_unit_from_ancillary_packet(uint16_t *py, uint16_t return tgt; } -uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words, -unsigned _count) +static uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words, + unsigned _count) { size_t i, len = (buf[5] & 0xff) + 6 + 1; uint8_t cdp_sum, rate; @@ -352,8 +352,8 @@ uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words, return cc; } -uint8_t *get_metadata(AVFormatContext *avctx, uint16_t *buf, size_t width, - uint8_t *tgt, size_t tgt_size, AVPacket *pkt) +static uint8_t *get_metadata(AVFormatContext *avctx, uint16_t *buf, size_t width, + uint8_t *tgt, size_t tgt_size, AVPacket *pkt) { decklink_cctx *cctx = (struct decklink_cctx *) avctx->priv_data; uint16_t *max_buf = buf + width; LGTM ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixing small memory leak when wrapping texture in AVD3D11FrameDescriptor
On 11/4/2017 9:43 PM, Greg Wessels wrote: --- libavutil/hwcontext_d3d11va.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libavutil/hwcontext_d3d11va.c b/libavutil/hwcontext_d3d11va.c index 52683b9..65dd665 100644 --- a/libavutil/hwcontext_d3d11va.c +++ b/libavutil/hwcontext_d3d11va.c @@ -123,6 +123,7 @@ static void d3d11va_frames_uninit(AVHWFramesContext *ctx) static void free_texture(void *opaque, uint8_t *data) { ID3D11Texture2D_Release((ID3D11Texture2D *)opaque); +av_free(data); } static AVBufferRef *wrap_texture_buf(ID3D11Texture2D *tex, int index) LGTM. I've also noticed a memory leak with hardware-accelerated D3D11 H.264 decoding, and this patch fixes the issue. The actual memory leak originates from wrap_texture_buf() in the same file from the following line: AVD3D11FrameDescriptor *desc = av_mallocz(sizeof(*desc)); and there will be one leak per surface per hardware decoding session. The number of surfaces can vary, but for H.264 and HEVC, it is equal to 20 (4 + 16). So, for H.264, there will be 20 leaks per decoding session. It would be helpful to see this committed soon. Additionally, I looked at the libav source base, and this issue exists there as well. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/1] avdevice/decklink_dec: Autodetect the video input format
See comments below. Aaron Levinson On 11/5/2017 4:49 PM, Marton Balint wrote: On Fri, 27 Oct 2017, Jeyapal, Karthick wrote: Please find the patch attached. Thanks, below some comments: From b18679b91a79f5e23a5ad23ae70f3862a34ddfb8 Mon Sep 17 00:00:00 2001 From: Karthick J <kjeya...@akamai.com> Date: Fri, 27 Oct 2017 12:00:23 +0530 Subject: [PATCH 1/1] avdevice/decklink_dec: Autodetect the video input format When -format_code is not specified autodetection will happen --- doc/indevs.texi | 1 + libavdevice/decklink_common.cpp | 33 ++- libavdevice/decklink_common.h | 8 + libavdevice/decklink_dec.cpp | 70 +++-- 4 files changed, 89 insertions(+), 23 deletions(-) diff --git a/doc/indevs.texi b/doc/indevs.texi index d308bbf..74bfcc5 100644 --- a/doc/indevs.texi +++ b/doc/indevs.texi @@ -238,6 +238,7 @@ This sets the input video format to the format given by the FourCC. To see the supported values of your device(s) use @option{list_formats}. Note that there is a FourCC @option{'pal '} that can also be used as @option{pal} (3 letters). +Default behavior is autodetection of the input video format ... video format, if the hardware supports it. @item bm_v210 This is a deprecated option, you can use @option{raw_format} instead. diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp index 2bd63ac..757f419 100644 --- a/libavdevice/decklink_common.cpp +++ b/libavdevice/decklink_common.cpp @@ -148,23 +148,10 @@ static DECKLINK_BOOL field_order_eq(enum AVFieldOrder field_order, BMDFieldDomin return false; } -int ff_decklink_set_format(AVFormatContext *avctx, - int width, int height, - int tb_num, int tb_den, - enum AVFieldOrder field_order, - decklink_direction_t direction, int num) -{ +void ff_decklink_set_duplex_mode(AVFormatContext *avctx) { struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data; struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx; - BMDDisplayModeSupport support; - IDeckLinkDisplayModeIterator *itermode; - IDeckLinkDisplayMode *mode; - int i = 1; HRESULT res; - - av_log(avctx, AV_LOG_DEBUG, "Trying to find mode for frame size %dx%d, frame timing %d/%d, field order %d, direction %d, mode number %d, format code %s\n", - width, height, tb_num, tb_den, field_order, direction, num, (cctx->format_code) ? cctx->format_code : "(unset)"); - if (ctx->duplex_mode) { DECKLINK_BOOL duplex_supported = false; @@ -181,6 +168,24 @@ int ff_decklink_set_format(AVFormatContext *avctx, av_log(avctx, AV_LOG_WARNING, "Unable to set duplex mode, because it is not supported.\n"); } } +} You factorized this out, but keep in mind that decklink_enc might also use this to set duplex mode. (even if there is no option to do that at the moment). Also it would make sense to rename the function and also put the input selection logic here, because even if you autodetect, you should select the input (sdi/hdmi/etc) first. This factorization should be a separate patch for easier review. + +int ff_decklink_set_format(AVFormatContext *avctx, + int width, int height, + int tb_num, int tb_den, + enum AVFieldOrder field_order, + decklink_direction_t direction, int num) +{ + struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data; + struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx; + BMDDisplayModeSupport support; + IDeckLinkDisplayModeIterator *itermode; + IDeckLinkDisplayMode *mode; + int i = 1; + HRESULT res; + + av_log(avctx, AV_LOG_DEBUG, "Trying to find mode for frame size %dx%d, frame timing %d/%d, field order %d, direction %d, mode number %d, format code %s\n", + width, height, tb_num, tb_den, field_order, direction, num, (cctx->format_code) ? cctx->format_code : "(unset)"); if (direction == DIRECTION_IN) { int ret; diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h index b6acb01..f38fc14 100644 --- a/libavdevice/decklink_common.h +++ b/libavdevice/decklink_common.h @@ -31,6 +31,12 @@ class decklink_output_callback; class decklink_input_callback; +typedef enum autodetect_state { + AUTODETECT_RESET = 0, Maybe something like AUTODETECT_INACTIVE is a better name? + AUTODETECT_START, + AUTODETECT_DONE, +} autodetect_state; + typedef struct AVPacketQueue { AVPacketList *first_pkt, *last_pkt; int nb_packets; @@ -95,6 +101,7 @@ struct decklink_ctx { pthread_mutex_t mutex; pthread_cond_t cond; int frames_buffer_availabl
Re: [FFmpeg-devel] [PATCH 1/2] configure: Rename wincrypt to advapi32 for consistency with existing uses of check_lib
On 10/31/2017 11:44 AM, Hendrik Leppkes wrote: On Tue, Oct 31, 2017 at 6:50 PM, Aaron Levinson <alevinsn_...@levland.net> wrote: Signed-off-by: Aaron Levinson <alevinsn_...@levland.net> --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index ea22d692b9..572299c9d3 100755 --- a/configure +++ b/configure @@ -3327,7 +3327,7 @@ avformat_deps="avcodec avutil" avformat_suggest="libm network zlib" avresample_deps="avutil" avresample_suggest="libm" -avutil_suggest="clock_gettime libm libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia wincrypt" +avutil_suggest="clock_gettime libm libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia advapi32" postproc_deps="avutil gpl" postproc_suggest="libm" swresample_deps="avutil" @@ -5839,7 +5839,7 @@ check_builtin stdatomic_h stdatomic.h "atomic_int foo, bar = ATOMIC_VAR_INIT(-1) check_lib ole32"windows.h"CoTaskMemFree-lole32 check_lib shell32 "windows.h shellapi.h" CommandLineToArgvW -lshell32 -check_lib wincrypt "windows.h wincrypt.h" CryptGenRandom -ladvapi32 +check_lib advapi32 "windows.h wincrypt.h" CryptGenRandom -ladvapi32 check_lib psapi"windows.h psapi.h"GetProcessMemoryInfo -lpsapi enabled appkit && check_apple_framework AppKit advapi32 contains many more things, while wincrypt refers to the name of the crypt header specifically, so wincrypt seems a bit more correct to me. - Hendrik I'm aware of this, but it seems to me either we use an approach that has many different check_lib statements for various API classes, even if the same library is used for each, or we have a single check_lib statement that picks a frequently used API from each library. For example, how did someone arrive at CoTaskMemFree for ole32 when there are plenty of other APIs implemented in ole32? The same goes for CommandLineToArgvW in shell32. I could add a general advapi32 check_lib statement in addition to wincrypt, but the result will be the same--the addition of a dependency on advapi32. Also, if the argument is, we want to make sure that we are using a specific version of advapi32 that supports CryptGenRandom, I could see that applying to a relatively new Win32 API, but CryptGenRandom has been around since Windows XP, and it is just as good as most APIs implemented in advapi32 for being the "principal" API associated with advapi32. Adding a dependency on a component named "wincrypt", when all we want is access to the Registry APIs, I think is a bit misleading. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] configure, fftools/Makefile: Copy .pdb files to bindir for MSVC builds for make install
When ffmpeg is built using MSVC, symbols are stored separately from executables and libraries in .pdb files. However, unlike with gcc-based builds, when make install is invoked, symbols, in the form of .pdb files, which are important for debugging, are not copied to bindir. This change corrects this deficiency for MSVC builds. Files are also uninstalled appropriately when make uninstall is exercised. Note: General idea for how to properly handle the copying of PDB files associated with programs suggested by Hendrik Leppkes. Comments: -- configure: a) Leverage already existing SLIB_INSTALL_EXTRA_SHLIB facility (which is already pretty specific to Windows) to add .pdb files, in addition to .lib files, for shared libraries to bindir during make install. b) Add PROG_INSTALL_EXTRA_BIN variable for MSVC builds and also add it to the section that causes it to be added to config.mak. This is used in Makefile to copy any additional files associated with programs. For MSVC, it is used to copy the pdb files associated with any executables that are built. Note that such executables are build with _g in the file name and are later copied to a filename without _g in the file name. As such, the PDB files have _g in the file name. -- fftools/Makefile: Enhance install-progs and uninstall-progs targets to handle PROG_INSTALL_EXTRA_BIN if defined. It uses a similar procedure as already in place for SLIB_INSTALL_EXTRA_SHLIB in library.mak. Signed-off-by: Aaron Levinson <alevinsn_...@levland.net> --- configure| 4 +++- fftools/Makefile | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 2ac6fed98a..feb86b2069 100755 --- a/configure +++ b/configure @@ -5101,9 +5101,10 @@ case $target_os in SLIB_CREATE_DEF_CMD='$(SRC_PATH)/compat/windows/makedef $(SUBDIR)lib$(NAME).ver $(OBJS) > $$(@:$(SLIBSUF)=.def)' SLIB_INSTALL_NAME='$(SLIBNAME_WITH_MAJOR)' SLIB_INSTALL_LINKS= -SLIB_INSTALL_EXTRA_SHLIB='$(SLIBNAME:$(SLIBSUF)=.lib)' +SLIB_INSTALL_EXTRA_SHLIB='$(SLIBNAME:$(SLIBSUF)=.lib) $(SLIBNAME_WITH_MAJOR:$(SLIBSUF)=.pdb)' SLIB_INSTALL_EXTRA_LIB='$(SLIBNAME_WITH_MAJOR:$(SLIBSUF)=.def)' SHFLAGS='-dll -def:$$(@:$(SLIBSUF)=.def) -implib:$(SUBDIR)$(SLIBNAME:$(SLIBSUF)=.lib)' +PROG_INSTALL_EXTRA_BIN='$(AVPROGS-yes:%=%$(PROGSSUF)_g.pdb)' enabled x86_64 && objformat="win64" || objformat="win32" ranlib=: enable dos_paths @@ -7034,6 +7035,7 @@ SLIB_INSTALL_NAME=${SLIB_INSTALL_NAME} SLIB_INSTALL_LINKS=${SLIB_INSTALL_LINKS} SLIB_INSTALL_EXTRA_LIB=${SLIB_INSTALL_EXTRA_LIB} SLIB_INSTALL_EXTRA_SHLIB=${SLIB_INSTALL_EXTRA_SHLIB} +PROG_INSTALL_EXTRA_BIN=${PROG_INSTALL_EXTRA_BIN} VERSION_SCRIPT_POSTPROCESS_CMD=${VERSION_SCRIPT_POSTPROCESS_CMD} SAMPLES:=${samples:-\$(FATE_SAMPLES)} NOREDZONE_FLAGS=$noredzone_flags diff --git a/fftools/Makefile b/fftools/Makefile index c867814a71..8c121b9762 100644 --- a/fftools/Makefile +++ b/fftools/Makefile @@ -47,11 +47,13 @@ install-progs-$(CONFIG_SHARED): install-libs install-progs: install-progs-yes $(AVPROGS) $(Q)mkdir -p "$(BINDIR)" $(INSTALL) -c -m 755 $(AVPROGS) "$(BINDIR)" + $(if $(PROG_INSTALL_EXTRA_BIN), $(INSTALL) -m 644 $(PROG_INSTALL_EXTRA_BIN) "$(BINDIR)") uninstall: uninstall-progs uninstall-progs: $(RM) $(addprefix "$(BINDIR)/", $(ALLAVPROGS)) + $(if $(PROG_INSTALL_EXTRA_BIN), $(RM) $(addprefix "$(BINDIR)/", $(PROG_INSTALL_EXTRA_BIN))) clean:: $(RM) $(ALLAVPROGS) $(ALLAVPROGS_G) $(CLEANSUFFIXES:%=fftools/%) -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] configure: Fix backup libmfx MSVC build by adding advapi32 library dependency
Signed-off-by: Aaron Levinson <alevinsn_...@levland.net> --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 572299c9d3..2ac6fed98a 100755 --- a/configure +++ b/configure @@ -5994,7 +5994,7 @@ enabled libkvazaar&& require_pkg_config libkvazaar "kvazaar >= 0.8.1" kv # pkg-config support. Instead, users should make sure that the build # can find the libraries and headers through other means. enabled libmfx&& { use_pkg_config libmfx libmfx "mfx/mfxvideo.h" MFXInit || - { require libmfx "mfx/mfxvideo.h" MFXInit -llibmfx && warn "using libmfx without pkg-config"; } } + { require libmfx "mfx/mfxvideo.h" MFXInit "-llibmfx $advapi32_extralibs" && warn "using libmfx without pkg-config"; } } enabled libmodplug&& require_pkg_config libmodplug libmodplug libmodplug/modplug.h ModPlug_Load enabled libmp3lame&& require "libmp3lame >= 3.98.3" lame/lame.h lame_set_VBR_quality -lmp3lame enabled libmysofa && require libmysofa "mysofa.h" mysofa_load -lmysofa $zlib_extralibs -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] configure: Rename wincrypt to advapi32 for consistency with existing uses of check_lib
Signed-off-by: Aaron Levinson <alevinsn_...@levland.net> --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index ea22d692b9..572299c9d3 100755 --- a/configure +++ b/configure @@ -3327,7 +3327,7 @@ avformat_deps="avcodec avutil" avformat_suggest="libm network zlib" avresample_deps="avutil" avresample_suggest="libm" -avutil_suggest="clock_gettime libm libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia wincrypt" +avutil_suggest="clock_gettime libm libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia advapi32" postproc_deps="avutil gpl" postproc_suggest="libm" swresample_deps="avutil" @@ -5839,7 +5839,7 @@ check_builtin stdatomic_h stdatomic.h "atomic_int foo, bar = ATOMIC_VAR_INIT(-1) check_lib ole32"windows.h"CoTaskMemFree-lole32 check_lib shell32 "windows.h shellapi.h" CommandLineToArgvW -lshell32 -check_lib wincrypt "windows.h wincrypt.h" CryptGenRandom -ladvapi32 +check_lib advapi32 "windows.h wincrypt.h" CryptGenRandom -ladvapi32 check_lib psapi"windows.h psapi.h"GetProcessMemoryInfo -lpsapi enabled appkit && check_apple_framework AppKit -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avdevice/decklink_dec: add support for receiving op47 teletext
On 7/3/2017 1:06 PM, Marton Balint wrote: On Mon, 3 Jul 2017, Aaron Levinson wrote: +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. Searching for "full HD" tends to indicate that term is usually associated with 1080p but not with 720p nor 1080i (for example, at http://www.pcmag.com/article2/0,2817,2413044,00.asp ). I think it would be clearer to call out the specific video modes that are supported. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avdevice/decklink_dec: add support for receiving op47 teletext
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. +av_log(avctx, AV_LOG_ERROR, "Too many OP47 teletext packets.\n"); +break; +} +} +} vanc->Release(); if (txt_buf - txt_buf0 > 1) { int stuffing_units = (4 - ((45 + txt_buf - txt_buf0) / 46) % 4) % 4; @@ -423,7 +526,6 @@ HRESULT decklink_input_callback::VideoInputFrameArrived( } } } -#endif if (avpacket_queue_put(>queue, ) < 0) { ++ctx->dropped; @@ -522,13 +624,6 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) ctx->draw_bars = cctx->draw_bars; cctx->ctx = ctx; -#if !CONFIG_LIBZVBI -if (ctx->teletext_lines) { -av_log(avctx, AV_LOG_ERROR, "Libzvbi support is needed for capturing teletext, please recompile FFmpeg.\n"); -return AVERROR(ENOSYS); -} -#endif - /* Check audio channel option for valid values: 2, 8 or 16 */ switch (cctx->audio_channels) { case 2: @@ -582,6 +677,14 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) } } +#if !CONFIG_LIBZVBI +if (ctx->teletext_lines && ctx->bmd_width == 720) { +av_log(avctx, AV_LOG_ERROR, "Libzvbi support is needed for capturing SD teletext, please recompile FFmpeg.\n"); +ret = AVERROR(ENOSYS); +goto error; +} +#endif + /* Setup streams. */ st = avformat_new_stream(avctx, NULL); if (!st) { Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avdevice/decklink_dec: add support for decoding teletext from 10bit ancillary data
ask = 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**)) == 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. +for (i = 6; i < 336; i++, line_mask <<= 1) { +uint8_t *buf; +if ((ctx->teletext_lines & line_mask) && vanc->GetBufferForVerticalBlankingLine(i, (void**)) == 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. +} +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. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] compat: LoadLibrary isn't available on UWP/WinRT
On 6/2/2017 7:11 AM, wm4 wrote: On Fri, 2 Jun 2017 15:29:07 +0200 Hugo Beauzée-Luyssen <h...@beauzee.fr> wrote: --- compat/w32dlfcn.h | 4 1 file changed, 4 insertions(+) diff --git a/compat/w32dlfcn.h b/compat/w32dlfcn.h index bc9bb8c9f5..308763be53 100644 --- a/compat/w32dlfcn.h +++ b/compat/w32dlfcn.h @@ -71,7 +71,11 @@ exit: #ifndef LOAD_LIBRARY_SEARCH_SYSTEM32 # define LOAD_LIBRARY_SEARCH_SYSTEM320x0800 #endif +#if WINAPI_FAMILY_PARTITION (WINAPI_PARTITION_DESKTOP) return LoadLibraryExA(name, NULL, LOAD_LIBRARY_SEARCH_APPLICATION_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32); +#else +return NULL; +#endif } #define dlopen(name, flags) win32_dlopen(name) #define dlclose FreeLibrary Isn't it that LoadLibraryW is available, just not the A version? According to https://msdn.microsoft.com/en-us/library/windows/apps/mt654039.aspx#_api-ms-win-core-libraryloader-l1-2-0.dll , both the A and W versions of LoadLibrary and LoadLibraryEx would seem to be available on UWP. However, at the same time, I don't see any of the LoadLibrary APIs listed at either https://msdn.microsoft.com/en-us/library/windows/apps/dn424765.aspx , https://docs.microsoft.com/en-us/uwp/win32-and-com/win32-apis , or https://docs.microsoft.com/en-us/uwp/win32-and-com/win32-extension-apis , so perhaps LoadLibrary and LoadLibraryEx aren't supported. So, there appears to be some conflicting information on MSDN. According to https://msdn.microsoft.com/en-us/library/mt186162.aspx , it is necessary to use LoadPackagedLibrary in UWP apps instead of LoadLibrary or LoadLibraryEx. According to the article, "These macros are only available in Windows SDK 8.1 and later, so if your code needs to compile with earlier versions of the Windows SDK or for other platforms besides Windows, then you should also consider the case that none of them are defined." I'm not aware of ffmpeg necessarily having a requirement that it be built with the Windows 8.1 SDK or later, and this lack of requirement would tend to indicate that the proposed patch isn't sufficient. Plus, there is no reason to #define LOAD_LIBRARY_SEARCH_SYSTEM32 if the code won't be built anyway. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure: If vfwcap_indev disabled, don't do vfw32 checks; if enabled, require vfw32
On 5/18/2017 9:21 AM, Hendrik Leppkes wrote: On Thu, May 18, 2017 at 3:18 PM, Michael Niedermayer <mich...@niedermayer.cc> wrote: On Tue, May 16, 2017 at 03:38:32PM -0700, Aaron Levinson wrote: Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 7980d92..49f91ac 100755 --- a/configure +++ b/configure @@ -6013,10 +6013,10 @@ check_header sys/videoio.h check_code cc sys/videoio.h "struct v4l2_frmsizeenum vfse; vfse.discrete.width = 0;" && enable_safe struct_v4l2_frmivalenum_discrete check_lib user32 "windows.h winuser.h" GetShellWindow -luser32 -check_lib vfw32 "windows.h vfw.h" capCreateCaptureWindow -lvfw32 +disabled vfwcap_indev || { require vfw32 "windows.h vfw.h" capCreateCaptureWindow -lvfw32 && # check that WM_CAP_DRIVER_CONNECT is defined to the proper value # w32api 3.12 had it defined wrong -check_cpp_condition vfw.h "WM_CAP_DRIVER_CONNECT > WM_USER" && enable vfwcap_defines + check_cpp_condition vfw.h "WM_CAP_DRIVER_CONNECT > WM_USER" && enable vfwcap_defines; } this breaks build on linux ERROR: vfw32 not found As discussed on IRC, other then the error on linux, overall I'm not sure what exactly this change improves? vfw detection seems to work just fine as is. Agree--thanks for looking at this. Please disregard this patch--it is incorrect in a couple of different ways. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure: If vfwcap_indev disabled, don't do vfw32 checks; if enabled, require vfw32
On 5/16/2017 4:23 PM, Steven Liu wrote: 2017-05-17 6:38 GMT+08:00 Aaron Levinson <alevi...@aracnet.com>: Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 7980d92..49f91ac 100755 --- a/configure +++ b/configure @@ -6013,10 +6013,10 @@ check_header sys/videoio.h check_code cc sys/videoio.h "struct v4l2_frmsizeenum vfse; vfse.discrete.width = 0;" && enable_safe struct_v4l2_frmivalenum_discrete check_lib user32 "windows.h winuser.h" GetShellWindow -luser32 -check_lib vfw32 "windows.h vfw.h" capCreateCaptureWindow -lvfw32 +disabled vfwcap_indev || { require vfw32 "windows.h vfw.h" capCreateCaptureWindow -lvfw32 && # check that WM_CAP_DRIVER_CONNECT is defined to the proper value # w32api 3.12 had it defined wrong -check_cpp_condition vfw.h "WM_CAP_DRIVER_CONNECT > WM_USER" && enable vfwcap_defines + check_cpp_condition vfw.h "WM_CAP_DRIVER_CONNECT > WM_USER" && enable vfwcap_defines; } indent problem? It's not--it is aligned with "require" on the earlier line, and that alignment is consistent with similar things in other parts of the file. I decided to leave the comment alone though, but I could indent the comment with the other lines if that makes more sense. Aaron ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] configure: If vfwcap_indev disabled, don't do vfw32 checks; if enabled, require vfw32
Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 7980d92..49f91ac 100755 --- a/configure +++ b/configure @@ -6013,10 +6013,10 @@ check_header sys/videoio.h check_code cc sys/videoio.h "struct v4l2_frmsizeenum vfse; vfse.discrete.width = 0;" && enable_safe struct_v4l2_frmivalenum_discrete check_lib user32 "windows.h winuser.h" GetShellWindow -luser32 -check_lib vfw32 "windows.h vfw.h" capCreateCaptureWindow -lvfw32 +disabled vfwcap_indev || { require vfw32 "windows.h vfw.h" capCreateCaptureWindow -lvfw32 && # check that WM_CAP_DRIVER_CONNECT is defined to the proper value # w32api 3.12 had it defined wrong -check_cpp_condition vfw.h "WM_CAP_DRIVER_CONNECT > WM_USER" && enable vfwcap_defines + check_cpp_condition vfw.h "WM_CAP_DRIVER_CONNECT > WM_USER" && enable vfwcap_defines; } check_type "dshow.h" IBaseFilter -- 2.10.1.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avutil/hwcontext_dxva2: Don't improperly free IDirect3DSurface9 objects
Add dxva2_pool_release_dummy() and use it in call to av_buffer_create() in dxva2_pool_alloc(). Prior to this change, av_buffer_create() was called with NULL for the third argument, which indicates that av_buffer_default_free() should be used to free the buffer's data. Eventually, it gets to buffer_pool_free() and calls buf->free() on a surface object (which is av_buffer_default_free()). This can result in a crash when the debug version of the C-runtime is used on Windows. While it doesn't appear to result in a crash when the release version of the C-runtime is used on Windows, it likely results in memory corruption, since av_free() is being called on memory that was allocated using IDirectXVideoAccelerationService::CreateSurface(). Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- libavutil/hwcontext_dxva2.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c index 4ed0d56..6c41788 100644 --- a/libavutil/hwcontext_dxva2.c +++ b/libavutil/hwcontext_dxva2.c @@ -121,6 +121,13 @@ static void dxva2_frames_uninit(AVHWFramesContext *ctx) } } +static void dxva2_pool_release_dummy(void *opaque, uint8_t *data) +{ +// important not to free anything here--data is a surface object +// associated with the call to CreateSurface(), and these surfaces are +// released in dxva2_frames_uninit() +} + static AVBufferRef *dxva2_pool_alloc(void *opaque, int size) { AVHWFramesContext *ctx = (AVHWFramesContext*)opaque; @@ -130,7 +137,7 @@ static AVBufferRef *dxva2_pool_alloc(void *opaque, int size) if (s->nb_surfaces_used < hwctx->nb_surfaces) { s->nb_surfaces_used++; return av_buffer_create((uint8_t*)s->surfaces_internal[s->nb_surfaces_used - 1], -sizeof(*hwctx->surfaces), NULL, 0, 0); +sizeof(*hwctx->surfaces), dxva2_pool_release_dummy, 0, 0); } return NULL; -- 2.10.1.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/hwcontext_dxva2: No longer improperly freeing IDirect3DSurface9 objects
Please disregard--I will submit a new patch with a better commit message. Aaron On 5/16/2017 4:09 AM, Aaron Levinson wrote: Purpose: No longer improperly freeing IDirect3DSurface9 objects in hwcontext_dxva2.c. Added dxva2_pool_release_dummy() and using it in call to av_buffer_create() in dxva2_pool_alloc(). Prior to this change, av_buffer_create() was called with NULL for the third argument, which indicates that av_buffer_default_free() should be used to free the buffer's data. Eventually, it gets to buffer_pool_free() and calls buf->free() on a surface object (which is av_buffer_default_free()). This can result in a crash when the debug version of the C-runtime is used on Windows. While it doesn't appear to result in a crash when the release version of the C-runtime is used on Windows, it likely results in memory corruption, since av_free() is being called on memory that was allocated using IDirectXVideoAccelerationService::CreateSurface(). Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- libavutil/hwcontext_dxva2.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c index 4ed0d56..6c41788 100644 --- a/libavutil/hwcontext_dxva2.c +++ b/libavutil/hwcontext_dxva2.c @@ -121,6 +121,13 @@ static void dxva2_frames_uninit(AVHWFramesContext *ctx) } } +static void dxva2_pool_release_dummy(void *opaque, uint8_t *data) +{ +// important not to free anything here--data is a surface object +// associated with the call to CreateSurface(), and these surfaces are +// released in dxva2_frames_uninit() +} + static AVBufferRef *dxva2_pool_alloc(void *opaque, int size) { AVHWFramesContext *ctx = (AVHWFramesContext*)opaque; @@ -130,7 +137,7 @@ static AVBufferRef *dxva2_pool_alloc(void *opaque, int size) if (s->nb_surfaces_used < hwctx->nb_surfaces) { s->nb_surfaces_used++; return av_buffer_create((uint8_t*)s->surfaces_internal[s->nb_surfaces_used - 1], -sizeof(*hwctx->surfaces), NULL, 0, 0); +sizeof(*hwctx->surfaces), dxva2_pool_release_dummy, 0, 0); } return NULL; ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avutil/hwcontext_dxva2: No longer improperly freeing IDirect3DSurface9 objects
Purpose: No longer improperly freeing IDirect3DSurface9 objects in hwcontext_dxva2.c. Added dxva2_pool_release_dummy() and using it in call to av_buffer_create() in dxva2_pool_alloc(). Prior to this change, av_buffer_create() was called with NULL for the third argument, which indicates that av_buffer_default_free() should be used to free the buffer's data. Eventually, it gets to buffer_pool_free() and calls buf->free() on a surface object (which is av_buffer_default_free()). This can result in a crash when the debug version of the C-runtime is used on Windows. While it doesn't appear to result in a crash when the release version of the C-runtime is used on Windows, it likely results in memory corruption, since av_free() is being called on memory that was allocated using IDirectXVideoAccelerationService::CreateSurface(). Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- libavutil/hwcontext_dxva2.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c index 4ed0d56..6c41788 100644 --- a/libavutil/hwcontext_dxva2.c +++ b/libavutil/hwcontext_dxva2.c @@ -121,6 +121,13 @@ static void dxva2_frames_uninit(AVHWFramesContext *ctx) } } +static void dxva2_pool_release_dummy(void *opaque, uint8_t *data) +{ +// important not to free anything here--data is a surface object +// associated with the call to CreateSurface(), and these surfaces are +// released in dxva2_frames_uninit() +} + static AVBufferRef *dxva2_pool_alloc(void *opaque, int size) { AVHWFramesContext *ctx = (AVHWFramesContext*)opaque; @@ -130,7 +137,7 @@ static AVBufferRef *dxva2_pool_alloc(void *opaque, int size) if (s->nb_surfaces_used < hwctx->nb_surfaces) { s->nb_surfaces_used++; return av_buffer_create((uint8_t*)s->surfaces_internal[s->nb_surfaces_used - 1], -sizeof(*hwctx->surfaces), NULL, 0, 0); +sizeof(*hwctx->surfaces), dxva2_pool_release_dummy, 0, 0); } return NULL; -- 2.10.1.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] FFMpeg GPL license Violations by Android apps
On 5/15/2017 1:48 PM, riya khanna wrote: We built a system to match mobile app binaries against an indexing database of signatures extracted across various popular OSS sources (e.g., ffmpeg). We should be able to share a draft of our design and implementation with you soon. Thanks! You indicated that each of these projects uses a GPL build of FFmpeg. Are they linking to a GPL build of FFmpeg for Android or are they merely shipping binaries and invoking the ffmpeg binary as a separate process? Or perhaps they have extracted source code from FFmpeg, whether licensed under the GPL or the LGPL, and are using that in their own project, which would also generally violate the terms of the GPL and/or LGPL. However, if they are using the approach of just launching ffmpeg in a separate process, I think that in general doesn't violate the GPL as long as it is clear where to get the source code that was used to build the FFmpeg binaries that were shipped with the app. Aaron Levinson On Mon, May 15, 2017 at 1:38 PM, Kieran Kunhya <kier...@obe.tv> wrote: Hi, Out of interest, how did you automate this process? Is it as simple as looking for the FFmpeg configure string? Regards, Kieran Kunhya On Mon, 15 May 2017 at 16:36 riya khanna <riyakhanna1...@gmail.com> wrote: Typo: We have inspected *1.6 million free* Google Playstore Android apps On Mon, May 15, 2017 at 11:24 AM, riya khanna <riyakhanna1...@gmail.com> wrote: Please find the list of apps here: https://docs.google.com/spreadsheets/d/1YnBHbsYIyGxQbjpaIbU4LFjha3hx8 3ciKLk7htVjpT4/edit?usp=sharing We have inspected 1.6 free Google Playstore Android apps and found over 4,300 apps violating GPL 2 or 3 (--enable-gpl) licensing terms. Let us know if you have any questions. Thanks! -FSFPolice On Thu, May 11, 2017 at 11:33 AM, Carl Eugen Hoyos <ceffm...@gmail.com wrote: 2017-05-11 15:55 GMT+02:00 riya khanna <riyakhanna1...@gmail.com>: We are a group of PhD students at Georgia Tech carrying out a research project that explores free software violations by mobile apps. We found that over 100 Android apps use GPL version of FFMpeg. We can share a list of all the apps if you are interested in details. Please let us know. You could send the list to this mailing list. (Please do not open 100 tickets on our bug tracker.) There's really not much we can do, Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video
On 5/12/2017 5:51 PM, Michael Niedermayer wrote: On Fri, May 12, 2017 at 01:19:43PM -0700, Aaron Levinson wrote: On 5/9/2017 11:56 AM, Michael Niedermayer wrote: On Thu, May 04, 2017 at 11:46:30PM -0700, Aaron Levinson wrote: I've provided a new version of the patch. When I created the first version of the patch on March 26th, this was the first patch that I submitted to ffmpeg, and some aspects were rough. I had indicated that the patch passed regression tests, but all I did was run "make fate", instead of "make fate SAMPLES=fate-suite/", and once I understood that I should use fate-suite, I discovered that some of the FATE tests failed with this patch. I fixed the issues with the patch, adjusted some comments, and adjusted the patch description text. I've confirmed that this patch passes all fate-suite tests for 64-bit MSVC on Windows and 64-bit gcc on Linux. Thanks, Aaron Levinson >From 85aa383f5753795652bae015f4fe91b016f7387e Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Thu, 4 May 2017 22:54:31 -0700 Subject: [PATCH] ffmpeg: Fixed bug with decoding interlaced video Purpose: Fixed bug in ffmpeg encountered when decoding interlaced video. In some cases, ffmpeg would treat it as progressive. As a result of the change, the issues with interlaced video are fixed. Detailed description of problem: Run the following command: "ffmpeg -i test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, test8_1080i.h264 is an H.264-encoded file with 1080i59.94 (interlaced). Prior to the patch, the following output is generated: Input #0, h264, from 'test8_1080i.h264': Duration: N/A, bitrate: N/A Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc Stream mapping: Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) Press [q] to stop, [?] for help Output #0, mpegts, to 'test8_1080i_mp2_2.ts': Metadata: encoder : Lavf57.72.100 Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc Metadata: encoder : Lavc57.92.100 mpeg2video which demonstrates the bug. The output should instead look like: Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc The bug is caused by the fact that reap_filters() calls init_output_stream(), which in turn calls check_init_output_file(), and this is all done prior to the first call to do_video_out(). An initial call to do_video_out() is necessary to populate the interlaced video information to the output stream's codecpar (mux_par->field_order in do_video_out()). However, check_init_output_file() calls avformat_write_header() prior to the initial call to do_video_out(), so field_order is populated too late, and the header is written with the default field_order value, progressive. Comments: -- ffmpeg.c: a) Enhanced init_output_stream() to take an additional input indicating whether or not check_init_output_file() should be called. There are other places that call init_output_stream(), and it was important to make sure that these continued to work properly. b) Adjusted reap_filters() such that, in the case that init_output_stream() is called, it indicates that it should not call check_init_output_file() in init_output_stream(). Instead, it makes the call to check_init_output_file() directly, but after it does the filtered frame setup and processing. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- ffmpeg.c | 61 + 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index e798d92277..45dbfafc04 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -1400,7 +1400,7 @@ static void do_video_stats(OutputStream *ost, int frame_size) } } -static int init_output_stream(OutputStream *ost, char *error, int error_len); +static int init_output_stream(OutputStream *ost, char *error, int error_len, int do_check_output_file); static void finish_output_stream(OutputStream *ost) { @@ -1415,6 +1415,8 @@ static void finish_output_stream(OutputStream *ost) } } +static int check_init_output_file(OutputFile *of, int file_index); should be close to the file top, together with similar prototypes Will do. [...] @@ -1521,6 +1526,44 @@ static int reap_filters(int flush) } av_frame_unref(filtered_frame); + +/* + * It is important to call check_init_output_file() here, after + * do_video_out() was called, instead of in init_output_stream(), + * as was done previously. + * If called from init_output_stream(), it is possible th
[FFmpeg-devel] [PATCH 2/2] ffmpeg: Made execution of reap_filters() more deterministic with respect to when headers are written
Purpose: Made execution of reap_filters() more deterministic with respect to when headers are written in relationship with the initialization of output streams and the processing of input audio and/or video frames. This change fixes a bug in ffmpeg encountered when decoding interlaced video. In some cases, ffmpeg would treat it as progressive. Detailed description of problem: Run the following command: "ffmpeg -i test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, test8_1080i.h264 is an H.264-encoded file with 1080i59.94 (interlaced). Prior to the patch, the following output is generated: Input #0, h264, from 'test8_1080i.h264': Duration: N/A, bitrate: N/A Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc Stream mapping: Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) Press [q] to stop, [?] for help Output #0, mpegts, to 'test8_1080i_mp2_2.ts': Metadata: encoder : Lavf57.72.100 Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc Metadata: encoder : Lavc57.92.100 mpeg2video which demonstrates the bug. The output stream should instead look like: Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc The bug is caused by the fact that reap_filters() calls init_output_stream(), which is then immediately followed by a call to check_init_output_file(), and this is all done prior to the first call to do_video_out(). An initial call to do_video_out() is necessary to populate the interlaced video information to the output stream's codecpar (mux_par->field_order in do_video_out()). However, check_init_output_file() calls avformat_write_header() prior to the initial call to do_video_out(), so field_order is populated too late, and the header is written with the default field_order value, progressive. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- ffmpeg.c | 43 --- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index 3cd45ba665..7b044b068c 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -1434,6 +1434,7 @@ static int reap_filters(int flush) AVFilterContext *filter; AVCodecContext *enc = ost->enc_ctx; int ret = 0; +int do_check_init_output_file = 0; if (!ost->filter || !ost->filter->graph->graph) continue; @@ -1448,9 +1449,7 @@ static int reap_filters(int flush) exit_program(1); } -ret = check_init_output_file(of, ost->file_index); -if (ret < 0) -exit_program(1); +do_check_init_output_file = 1; } if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { @@ -1526,6 +1525,44 @@ static int reap_filters(int flush) } av_frame_unref(filtered_frame); + +/* + * It is important to call check_init_output_file() here, after + * do_video_out() was called, instead of in init_output_stream(), + * as was done previously. + * If called from init_output_stream(), it is possible that not + * everything will have been fully initialized by the time that + * check_init_output_file() is called, and if + * check_init_output_file() determines that all output streams + * have been initialized, it will write the header. An example + * of initialization that depends on do_video_out() being called + * at least once is the specification of interlaced video, which + * happens in do_video_out(). This is particularly relevant when + * decoding--without processing a video frame, the interlaced + * video setting is not propagated before the header is written, + * and that causes problems. + * TODO: should probably handle interlaced video differently + * and not depend on it being setup in do_video_out(). Another + * solution would be to set it up once by examining the input + * header. + */ +if (do_check_init_output_file) { +ret = check_init_output_file(of, ost->file_index); +if (ret < 0) +exit_program(1); +do_check_init_output_file = 0; +} +} + +/* + * Can't wait too long to call check_init_output_file(). + * Otherwise, bad things start to occur. + * If didn't do it earlier, do it by the time it gets here. + */ +if (do_check_init_output_file) { +ret = che
[FFmpeg-devel] [PATCH 1/2] ffmpeg: Separated check_init_output_file() from init_output_stream()
Purpose: Separated the call to check_init_output_file() from init_output_stream(), and now check_init_output_file() is called by the parent after calling init_output_stream(). Additionally, moved some static function declarations to the beginning of the file. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- ffmpeg.c | 27 +++ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index e798d92277..3cd45ba665 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -141,6 +141,9 @@ AVIOContext *progress_avio = NULL; static uint8_t *subtitle_out; +static int init_output_stream(OutputStream *ost, char *error, int error_len); +static int check_init_output_file(OutputFile *of, int file_index); + InputStream **input_streams = NULL; intnb_input_streams = 0; InputFile **input_files = NULL; @@ -1400,8 +1403,6 @@ static void do_video_stats(OutputStream *ost, int frame_size) } } -static int init_output_stream(OutputStream *ost, char *error, int error_len); - static void finish_output_stream(OutputStream *ost) { OutputFile *of = output_files[ost->file_index]; @@ -1446,6 +1447,10 @@ static int reap_filters(int flush) ost->file_index, ost->index, error); exit_program(1); } + +ret = check_init_output_file(of, ost->file_index); +if (ret < 0) +exit_program(1); } if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { @@ -1894,6 +1899,10 @@ static void flush_encoders(void) ost->file_index, ost->index, error); exit_program(1); } + +ret = check_init_output_file(of, ost->file_index); +if (ret < 0) +exit_program(1); } if (enc->codec_type == AVMEDIA_TYPE_AUDIO && enc->frame_size <= 1) @@ -3564,10 +3573,6 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len) ost->initialized = 1; -ret = check_init_output_file(output_files[ost->file_index], ost->file_index); -if (ret < 0) -return ret; - return ret; } @@ -3629,11 +3634,17 @@ static int transcode_init(void) /* open each encoder */ for (i = 0; i < nb_output_streams; i++) { +ost = output_streams[i]; // skip streams fed from filtergraphs until we have a frame for them -if (output_streams[i]->filter) +if (ost->filter) continue; -ret = init_output_stream(output_streams[i], error, sizeof(error)); +ret = init_output_stream(ost, error, sizeof(error)); +if (ret < 0) +goto dump_format; + +ret = check_init_output_file(output_files[ost->file_index], + ost->file_index); if (ret < 0) goto dump_format; } -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] ffmpeg: Separated check_init_output_file() from, init_output_stream()
Please disregard--I messed up the subject. I'll send a new one. Aaron Levinson On 5/12/2017 1:22 PM, Aaron Levinson wrote: Purpose: Separated the call to check_init_output_file() from init_output_stream(), and now check_init_output_file() is called by the parent after calling init_output_stream(). Additionally, moved some static function declarations to the beginning of the file. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- ffmpeg.c | 27 +++ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index e798d92277..3cd45ba665 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -141,6 +141,9 @@ AVIOContext *progress_avio = NULL; static uint8_t *subtitle_out; +static int init_output_stream(OutputStream *ost, char *error, int error_len); +static int check_init_output_file(OutputFile *of, int file_index); + InputStream **input_streams = NULL; intnb_input_streams = 0; InputFile **input_files = NULL; @@ -1400,8 +1403,6 @@ static void do_video_stats(OutputStream *ost, int frame_size) } } -static int init_output_stream(OutputStream *ost, char *error, int error_len); - static void finish_output_stream(OutputStream *ost) { OutputFile *of = output_files[ost->file_index]; @@ -1446,6 +1447,10 @@ static int reap_filters(int flush) ost->file_index, ost->index, error); exit_program(1); } + +ret = check_init_output_file(of, ost->file_index); +if (ret < 0) +exit_program(1); } if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { @@ -1894,6 +1899,10 @@ static void flush_encoders(void) ost->file_index, ost->index, error); exit_program(1); } + +ret = check_init_output_file(of, ost->file_index); +if (ret < 0) +exit_program(1); } if (enc->codec_type == AVMEDIA_TYPE_AUDIO && enc->frame_size <= 1) @@ -3564,10 +3573,6 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len) ost->initialized = 1; -ret = check_init_output_file(output_files[ost->file_index], ost->file_index); -if (ret < 0) -return ret; - return ret; } @@ -3629,11 +3634,17 @@ static int transcode_init(void) /* open each encoder */ for (i = 0; i < nb_output_streams; i++) { +ost = output_streams[i]; // skip streams fed from filtergraphs until we have a frame for them -if (output_streams[i]->filter) +if (ost->filter) continue; -ret = init_output_stream(output_streams[i], error, sizeof(error)); +ret = init_output_stream(ost, error, sizeof(error)); +if (ret < 0) +goto dump_format; + +ret = check_init_output_file(output_files[ost->file_index], + ost->file_index); if (ret < 0) goto dump_format; } ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] ffmpeg: Separated check_init_output_file() from, init_output_stream()
Purpose: Separated the call to check_init_output_file() from init_output_stream(), and now check_init_output_file() is called by the parent after calling init_output_stream(). Additionally, moved some static function declarations to the beginning of the file. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- ffmpeg.c | 27 +++ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index e798d92277..3cd45ba665 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -141,6 +141,9 @@ AVIOContext *progress_avio = NULL; static uint8_t *subtitle_out; +static int init_output_stream(OutputStream *ost, char *error, int error_len); +static int check_init_output_file(OutputFile *of, int file_index); + InputStream **input_streams = NULL; intnb_input_streams = 0; InputFile **input_files = NULL; @@ -1400,8 +1403,6 @@ static void do_video_stats(OutputStream *ost, int frame_size) } } -static int init_output_stream(OutputStream *ost, char *error, int error_len); - static void finish_output_stream(OutputStream *ost) { OutputFile *of = output_files[ost->file_index]; @@ -1446,6 +1447,10 @@ static int reap_filters(int flush) ost->file_index, ost->index, error); exit_program(1); } + +ret = check_init_output_file(of, ost->file_index); +if (ret < 0) +exit_program(1); } if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { @@ -1894,6 +1899,10 @@ static void flush_encoders(void) ost->file_index, ost->index, error); exit_program(1); } + +ret = check_init_output_file(of, ost->file_index); +if (ret < 0) +exit_program(1); } if (enc->codec_type == AVMEDIA_TYPE_AUDIO && enc->frame_size <= 1) @@ -3564,10 +3573,6 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len) ost->initialized = 1; -ret = check_init_output_file(output_files[ost->file_index], ost->file_index); -if (ret < 0) -return ret; - return ret; } @@ -3629,11 +3634,17 @@ static int transcode_init(void) /* open each encoder */ for (i = 0; i < nb_output_streams; i++) { +ost = output_streams[i]; // skip streams fed from filtergraphs until we have a frame for them -if (output_streams[i]->filter) +if (ost->filter) continue; -ret = init_output_stream(output_streams[i], error, sizeof(error)); +ret = init_output_stream(ost, error, sizeof(error)); +if (ret < 0) +goto dump_format; + +ret = check_init_output_file(output_files[ost->file_index], + ost->file_index); if (ret < 0) goto dump_format; } -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video
On 5/9/2017 11:56 AM, Michael Niedermayer wrote: On Thu, May 04, 2017 at 11:46:30PM -0700, Aaron Levinson wrote: I've provided a new version of the patch. When I created the first version of the patch on March 26th, this was the first patch that I submitted to ffmpeg, and some aspects were rough. I had indicated that the patch passed regression tests, but all I did was run "make fate", instead of "make fate SAMPLES=fate-suite/", and once I understood that I should use fate-suite, I discovered that some of the FATE tests failed with this patch. I fixed the issues with the patch, adjusted some comments, and adjusted the patch description text. I've confirmed that this patch passes all fate-suite tests for 64-bit MSVC on Windows and 64-bit gcc on Linux. Thanks, Aaron Levinson From 85aa383f5753795652bae015f4fe91b016f7387e Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Thu, 4 May 2017 22:54:31 -0700 Subject: [PATCH] ffmpeg: Fixed bug with decoding interlaced video Purpose: Fixed bug in ffmpeg encountered when decoding interlaced video. In some cases, ffmpeg would treat it as progressive. As a result of the change, the issues with interlaced video are fixed. Detailed description of problem: Run the following command: "ffmpeg -i test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, test8_1080i.h264 is an H.264-encoded file with 1080i59.94 (interlaced). Prior to the patch, the following output is generated: Input #0, h264, from 'test8_1080i.h264': Duration: N/A, bitrate: N/A Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc Stream mapping: Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) Press [q] to stop, [?] for help Output #0, mpegts, to 'test8_1080i_mp2_2.ts': Metadata: encoder : Lavf57.72.100 Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc Metadata: encoder : Lavc57.92.100 mpeg2video which demonstrates the bug. The output should instead look like: Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc The bug is caused by the fact that reap_filters() calls init_output_stream(), which in turn calls check_init_output_file(), and this is all done prior to the first call to do_video_out(). An initial call to do_video_out() is necessary to populate the interlaced video information to the output stream's codecpar (mux_par->field_order in do_video_out()). However, check_init_output_file() calls avformat_write_header() prior to the initial call to do_video_out(), so field_order is populated too late, and the header is written with the default field_order value, progressive. Comments: -- ffmpeg.c: a) Enhanced init_output_stream() to take an additional input indicating whether or not check_init_output_file() should be called. There are other places that call init_output_stream(), and it was important to make sure that these continued to work properly. b) Adjusted reap_filters() such that, in the case that init_output_stream() is called, it indicates that it should not call check_init_output_file() in init_output_stream(). Instead, it makes the call to check_init_output_file() directly, but after it does the filtered frame setup and processing. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- ffmpeg.c | 61 + 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index e798d92277..45dbfafc04 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -1400,7 +1400,7 @@ static void do_video_stats(OutputStream *ost, int frame_size) } } -static int init_output_stream(OutputStream *ost, char *error, int error_len); +static int init_output_stream(OutputStream *ost, char *error, int error_len, int do_check_output_file); static void finish_output_stream(OutputStream *ost) { @@ -1415,6 +1415,8 @@ static void finish_output_stream(OutputStream *ost) } } +static int check_init_output_file(OutputFile *of, int file_index); should be close to the file top, together with similar prototypes Will do. [...] @@ -1521,6 +1526,44 @@ static int reap_filters(int flush) } av_frame_unref(filtered_frame); + +/* + * It is important to call check_init_output_file() here, after + * do_video_out() was called, instead of in init_output_stream(), + * as was done previously. + * If called from init_output_stream(), it is possible that not + * everything will have been fully initialized by the time that
Re: [FFmpeg-devel] [PATCH] configure: jni no longer requires -ldl
On 5/12/2017 11:34 AM, Aman Gupta wrote: From: Aman Gupta <a...@tmm1.net> this dependency was removed in 33d69a90085d30af8a292d9364b835a26565d6b9 --- configure | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/configure b/configure index 5ae5227868..ba33241a45 100755 --- a/configure +++ b/configure @@ -5766,8 +5766,7 @@ enabled decklink && { { check_header DeckLinkAPI.h || die "ERROR: DeckL enabled frei0r&& { check_header frei0r.h || die "ERROR: frei0r.h header not found"; } enabled gmp && require gmp gmp.h mpz_export -lgmp enabled gnutls&& require_pkg_config gnutls gnutls/gnutls.h gnutls_global_init -enabled jni && { [ $target_os = "android" ] && check_header jni.h && enabled pthreads && - check_lib jni "dlfcn.h" dlopen -ldl || die "ERROR: jni not found"; } +enabled jni && { [ $target_os = "android" ] && check_header jni.h && enabled pthreads || die "ERROR: jni not found"; } enabled ladspa&& { check_header ladspa.h || die "ERROR: ladspa.h header not found"; } enabled libiec61883 && require libiec61883 libiec61883/iec61883.h iec61883_cmp_connect -lraw1394 -lavc1394 -lrom1394 -liec61883 enabled libass&& require_pkg_config libass ass/ass.h ass_library_init LGTM--I see that the use of dl was eliminated in 33d69a9 (at https://github.com/FFmpeg/FFmpeg/commit/33d69a90085d30af8a292d9364b835a26565d6b9 ). Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: move old_filename free operation earlier
On 5/10/2017 3:33 AM, Steven Liu wrote: Suggested-by: Aaron Levinson <alevi...@aracnet.com> Signed-off-by: Steven Liu <l...@chinaffmpeg.org> --- libavformat/hlsenc.c |3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 221089c..7ed121a 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -1549,14 +1549,13 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt) sls_flag_file_rename(hls, old_filename); ret = hls_start(s); } +av_free(old_filename); if (ret < 0) { -av_free(old_filename); return ret; } if ((ret = hls_window(s, 0)) < 0) { -av_free(old_filename); return ret; } } LGTM Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: fix memleak
On 5/8/2017 3:35 AM, Steven Liu wrote: fix memleak bug, when all the process is normal, just free old_filename Signed-off-by: Steven Liu <l...@chinaffmpeg.org> --- libavformat/hlsenc.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 221089c..d62d5b8 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -1559,6 +1559,8 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt) av_free(old_filename); return ret; } + +av_free(old_filename); This looks fine, but, if possible, it would be preferable to free the memory earlier if it is no longer needed. I didn't study the code sufficiently to determine this with certainty, but it is possible that old_filename is no longer needed after line 1551. In that case, free it earlier and eliminate any subsequent calls to av_free(old_filename). } ret = ff_write_chained(oc, stream_index, pkt, s, 0); Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: support TAG span multiple lines when parse playlist
Based on a conversation that I had on IRC with Martin Storsjö, I misinterpreted the Apple documentation, and the only reason why '\' shows up in these documents is so that lines won't appear too long, particularly in the RFC. According to Martin, Apple's tools can't handle .m3u8 files that use '\' for multi-line TAG spans, and in addition, there is no mention of '\' anywhere in the actual grammar documented in the specification. So, it seems that that this patch isn't needed. Aaron Levinson On 5/9/2017 1:01 PM, Aaron Levinson wrote: I would rewrite the commit message as: "avformat/hlsenc: support multi-line TAG spans when parsing a playlist". Also, I thought you were going to have a common implementation for both hlsenc and hls? There are a duplicate implementations of read_chomp_line() in hls.c, hlsenc.c, and hlsproto.c. This probably ought to be done in another patch, and I suggest making this functionality generic by moving to internal.h/aviobuf.c. The function could be called ff_get_multi_line_span(). On 5/5/2017 9:50 AM, Steven Liu wrote: refer to: https://developer.apple.com/library/content/technotes/tn2288/_index.html Note, the actual specification can be found at https://tools.ietf.org/html/draft-pantos-http-live-streaming-19 . This makes it clear how a '\' character is used to extend a tag declaration to the next line. I recommend referring to the draft spec instead of the link that I previously posted. support to parse the EXT-X-KEY span multiple lines: #EXTM3U #EXT-X-VERSION:3 #EXT-X-TARGETDURATION:10 #EXT-X-MEDIA-SEQUENCE:0 #EXT-X-KEY:METHOD=AES-128,URI="/file.key", \ IV=0x498c8325965f907960e3d94d20c4d138 #EXTINF:10.00, out0.ts #EXTINF:8.64, out1.ts #EXTINF:9.16, out2.ts Reported-by: Aaron Levinson <alevi...@aracnet.com> Signed-off-by: Steven Liu <l...@chinaffmpeg.org> --- libavformat/hlsenc.c | 21 +++-- 1 files changed, 19 insertions(+), 2 deletions(-) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 221089c..c167624 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -500,10 +500,27 @@ static int hls_encryption_start(AVFormatContext *s) static int read_chomp_line(AVIOContext *s, char *buf, int maxlen) { -int len = ff_get_line(s, buf, maxlen); +int len = 0; Should probably move the setting of len to 0 below the label (or within the while loop as I suggest later) to make it clear that the last value isn't relevant for further iterations of the loop. It is fine to declare len outside of the loop, although I think it would be cleaner if you declared it within the loop and adjusted the code accordingly to only increment total_len within the loop. Obviously, the declaration within the loop is only possible if you convert to a while loop. +int total_len = 0; + +re_get_line: This can easily be done with a while loop instead of using goto. In fact, what you are doing is basically a while loop, so I think it would be appropriate to do this as a while loop. +if (maxlen > total_len) { This is not quite right. According to the documentation for ff_get_line(), the length returned is "the length of the string written in the buffer, not including the final \\0". If, say, with the first call to ff_get_line(), it fills up the entire buffer, the length returned will be maxlen - 1. If this is a multi-line span, you'll iterate through the loop, see maxlen > total_len, and try again. Should likely be "if ((maxlen - 1) > total_len) {". +len = ff_get_line(s, buf, maxlen - total_len); This line is correct, since you do want to use the total buffer space for the call to ff_get_line(). +} else { +av_log(s, AV_LOG_WARNING, "The tag is too long, context only can get %s\n", buf); For this log message to work properly for multi-line spans, need to do char *buf2 = buf and work with buf2. With your current patch, if there is a multi-line span, buf will not point to the original, and the log message will incomplete in that case. +return maxlen; +} while (len > 0 && av_isspace(buf[len - 1])) buf[--len] = '\0'; This whitespace removal while loop won't do much in the case of a multi-line span. According to the spec, it is important to remove any trailing whitespace before the '\\': "A '\' is used to indicate that the tag continues on the following line with whitespace removed." While in the examples in the spec it might not be needed, a multi-line span could look something like the following: #EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="aac",NAME="Commentary", DEF \ AULT=NO,AUTOSELECT=NO,LANGUAGE="en", \ URI="commentary/audio-only.m3u8" As implemented in your patch, the whitespace removal code won't be triggered in the case that the last character in the line is a '\\', because in that case,
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: support TAG span multiple lines when parse playlist
I would rewrite the commit message as: "avformat/hlsenc: support multi-line TAG spans when parsing a playlist". Also, I thought you were going to have a common implementation for both hlsenc and hls? There are a duplicate implementations of read_chomp_line() in hls.c, hlsenc.c, and hlsproto.c. This probably ought to be done in another patch, and I suggest making this functionality generic by moving to internal.h/aviobuf.c. The function could be called ff_get_multi_line_span(). On 5/5/2017 9:50 AM, Steven Liu wrote: refer to: https://developer.apple.com/library/content/technotes/tn2288/_index.html Note, the actual specification can be found at https://tools.ietf.org/html/draft-pantos-http-live-streaming-19 . This makes it clear how a '\' character is used to extend a tag declaration to the next line. I recommend referring to the draft spec instead of the link that I previously posted. support to parse the EXT-X-KEY span multiple lines: #EXTM3U #EXT-X-VERSION:3 #EXT-X-TARGETDURATION:10 #EXT-X-MEDIA-SEQUENCE:0 #EXT-X-KEY:METHOD=AES-128,URI="/file.key", \ IV=0x498c8325965f907960e3d94d20c4d138 #EXTINF:10.00, out0.ts #EXTINF:8.64, out1.ts #EXTINF:9.16, out2.ts Reported-by: Aaron Levinson <alevi...@aracnet.com> Signed-off-by: Steven Liu <l...@chinaffmpeg.org> --- libavformat/hlsenc.c | 21 +++-- 1 files changed, 19 insertions(+), 2 deletions(-) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 221089c..c167624 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -500,10 +500,27 @@ static int hls_encryption_start(AVFormatContext *s) static int read_chomp_line(AVIOContext *s, char *buf, int maxlen) { -int len = ff_get_line(s, buf, maxlen); +int len = 0; Should probably move the setting of len to 0 below the label (or within the while loop as I suggest later) to make it clear that the last value isn't relevant for further iterations of the loop. It is fine to declare len outside of the loop, although I think it would be cleaner if you declared it within the loop and adjusted the code accordingly to only increment total_len within the loop. Obviously, the declaration within the loop is only possible if you convert to a while loop. +int total_len = 0; + +re_get_line: This can easily be done with a while loop instead of using goto. In fact, what you are doing is basically a while loop, so I think it would be appropriate to do this as a while loop. +if (maxlen > total_len) { This is not quite right. According to the documentation for ff_get_line(), the length returned is "the length of the string written in the buffer, not including the final \\0". If, say, with the first call to ff_get_line(), it fills up the entire buffer, the length returned will be maxlen - 1. If this is a multi-line span, you'll iterate through the loop, see maxlen > total_len, and try again. Should likely be "if ((maxlen - 1) > total_len) {". +len = ff_get_line(s, buf, maxlen - total_len); This line is correct, since you do want to use the total buffer space for the call to ff_get_line(). +} else { +av_log(s, AV_LOG_WARNING, "The tag is too long, context only can get %s\n", buf); For this log message to work properly for multi-line spans, need to do char *buf2 = buf and work with buf2. With your current patch, if there is a multi-line span, buf will not point to the original, and the log message will incomplete in that case. +return maxlen; +} while (len > 0 && av_isspace(buf[len - 1])) buf[--len] = '\0'; This whitespace removal while loop won't do much in the case of a multi-line span. According to the spec, it is important to remove any trailing whitespace before the '\\': "A '\' is used to indicate that the tag continues on the following line with whitespace removed." While in the examples in the spec it might not be needed, a multi-line span could look something like the following: #EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="aac",NAME="Commentary", DEF \ AULT=NO,AUTOSELECT=NO,LANGUAGE="en", \ URI="commentary/audio-only.m3u8" As implemented in your patch, the whitespace removal code won't be triggered in the case that the last character in the line is a '\\', because in that case, it will immediately fail the check and move on to the "if (buf[len - 1] == '\\') {" line. And with the above example, you won't get the desired result. But, you could in theory have whitespace both before _and_ after the '\', and to handle that correctly, you'll want the whitespace removal loop both before and after the '\\' detection code. -return len; + +if (buf[len - 1] == '\\') { +buf += len - 1; I suggest using a temporary variable for increment purposes. This will
Re: [FFmpeg-devel] [PATCH] videotoolbox: add hwcontext support
Are you also planning to change ffmpeg_videotoolbox.c? See below for more comments. Aaron Levinson On 5/2/2017 8:26 PM, wm4 wrote: This adds tons of code for no other benefit than making VideoToolbox support conform with the new hwaccel API (using hw_device_ctx and hw_frames_ctx). Since VideoToolbox decoding does not actually require the user to allocate frames, the new code does mostly nothing. One benefit is that ffmpeg_videotoolbox.c can be dropped once generic hwaccel support for ffmpeg.c is merged from Libav. Does not consider VDA or VideoToolbox encoding. Fun fact: the frame transfer functions are copied from vaapi, as the mapping makes copying generic boilerplate. Mapping itself is not exported by the VT code, because I don't know how to test. TODO: API bumps --- doc/APIchanges | 8 ++ libavcodec/vda_vt_internal.h | 7 ++ libavcodec/videotoolbox.c | 186 ++-- libavutil/Makefile | 3 + libavutil/hwcontext.c | 3 + libavutil/hwcontext.h | 1 + libavutil/hwcontext_internal.h | 1 + libavutil/hwcontext_videotoolbox.c | 243 + libavutil/hwcontext_videotoolbox.h | 54 + 9 files changed, 496 insertions(+), 10 deletions(-) create mode 100644 libavutil/hwcontext_videotoolbox.c create mode 100644 libavutil/hwcontext_videotoolbox.h diff --git a/doc/APIchanges b/doc/APIchanges index fcd3423d58..71f5563f03 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,14 @@ libavutil: 2015-08-28 Note that the APIchanges part prevents the entire patch from applying, but that's to be expected. API changes, most recent first: +2017-05-03 - xx - lavc 57.xx.100 - avcodec.h + VideoToolbox hardware accelerated decoding now supports the new hwaccel API, + which can create the decoder context and allocate hardware frame automatically. + See AVCodecContext.hw_device_ctx and AVCodecContext.hw_frames_ctx. I'd change the first sentence as follows: "VideoToolbox hardware-accelerated decoding now supports the new hwaccel API, which can create the decoder context and allocate hardware frames automatically." Changes are "hardware accelerated" -> "hardware-accelerated" and "hardware frame automatically" -> "hardware frames automatically". + +2017-05-03 - xx - lavu 57.xx.100 - hwcontext.h + Add AV_HWDEVICE_TYPE_VIDEOTOOLBOX and implementation. + 2017-04-11 - 8378466507 - lavu 55.61.100 - avstring.h Add av_strireplace(). diff --git a/libavcodec/vda_vt_internal.h b/libavcodec/vda_vt_internal.h index 9ff63ccc52..e55a813899 100644 --- a/libavcodec/vda_vt_internal.h +++ b/libavcodec/vda_vt_internal.h @@ -40,6 +40,13 @@ typedef struct VTContext { // The core video buffer CVImageBufferRefframe; + +// Current dummy frames context (depends on exact CVImageBufferRef params). +struct AVBufferRef *cached_hw_frames_ctx; + +// Non-NULL if the new hwaccel API is used. This is only a separate struct +// to ease compatibility with the old API. +struct AVVideotoolboxContext *vt_ctx; } VTContext; int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame); diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 67adad53ed..910ac25ea7 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -23,11 +23,13 @@ #include "config.h" #if CONFIG_VIDEOTOOLBOX # include "videotoolbox.h" +# include "libavutil/hwcontext_videotoolbox.h" #else # include "vda.h" #endif #include "vda_vt_internal.h" #include "libavutil/avutil.h" +#include "libavutil/hwcontext.h" #include "bytestream.h" #include "h264dec.h" #include "mpegvideo.h" @@ -188,6 +190,79 @@ int ff_videotoolbox_uninit(AVCodecContext *avctx) } #if CONFIG_VIDEOTOOLBOX +// Return the AVVideotoolboxContext that matters currently. Where it comes from +// depends on the API used. +static AVVideotoolboxContext *videotoolbox_get_context(AVCodecContext *avctx) +{ +// Somewhat tricky because the API user can call av_videotoolbox_default_free() +// at any time. Comment will make more sense if "API" is dropped from the sentence. +if (avctx->internal && avctx->internal->hwaccel_priv_data) { +VTContext *vtctx = avctx->internal->hwaccel_priv_data; +if (vtctx->vt_ctx) +return vtctx->vt_ctx; +} From your comment, and your answers to my questions on IRC, it is clear that these various checks are only needed for the case that av_videotoolbox_default_free() may be called after the codec is closed. However, this situation isn't relevant for most of the functions in your patch that call videotoolbox_get_content(). I suggest moving this check
[FFmpeg-devel] [PATCH 2/2] qsvenc: Make sure the interlaced encoding works
From 30eb78bac7bd92b0c085ba6873341e8319072acc Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 5 May 2017 21:31:30 -0700 Subject: [PATCH 2/2] qsvenc: Make sure the interlaced encoding works Purpose: qsvenc: make sure that interlaced encoding works. Also, reduce the vertical alignment constraint when possible to reduce memory usage. Note: Most of this code used to be present in ffmpeg and was eliminated in revision 1f26a23 on Oct. 31, 2016 (qsv: Merge libav implementation, at https://github.com/FFmpeg/FFmpeg/commit/1f26a231bb065276cd80ce02957c759f3197 edfa#diff-7d84a34d58597bb7aa4b8239dca1f9f8). Already applied to libav. Reviewed-by: Luca Barbato <lu_z...@gentoo.org> (cherry picked from commit 8fd8f91e47f33cd82371a97ac81afc476144964f) Signed-off-by: Mark Thompson <s...@jkqxz.net> Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- libavcodec/qsvenc.c | 29 +++-- libavcodec/qsvenc.h | 1 + 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index 68d4f5edd0..57bc83a47f 100644 --- a/libavcodec/qsvenc.c +++ b/libavcodec/qsvenc.c @@ -358,8 +358,6 @@ static int init_video_param(AVCodecContext *avctx, QSVEncContext *q) return AVERROR_BUG; q->param.mfx.CodecId = ret; -q->width_align = avctx->codec_id == AV_CODEC_ID_HEVC ? 32 : 16; - if (avctx->level > 0) q->param.mfx.CodecLevel = avctx->level; @@ -381,20 +379,39 @@ static int init_video_param(AVCodecContext *avctx, QSVEncContext *q) ff_qsv_map_pixfmt(sw_format, >param.mfx.FrameInfo.FourCC); -q->param.mfx.FrameInfo.Width = FFALIGN(avctx->width, q->width_align); -q->param.mfx.FrameInfo.Height = FFALIGN(avctx->height, 32); q->param.mfx.FrameInfo.CropX = 0; q->param.mfx.FrameInfo.CropY = 0; q->param.mfx.FrameInfo.CropW = avctx->width; q->param.mfx.FrameInfo.CropH = avctx->height; q->param.mfx.FrameInfo.AspectRatioW = avctx->sample_aspect_ratio.num; q->param.mfx.FrameInfo.AspectRatioH = avctx->sample_aspect_ratio.den; -q->param.mfx.FrameInfo.PicStruct = MFX_PICSTRUCT_PROGRESSIVE; q->param.mfx.FrameInfo.ChromaFormat = MFX_CHROMAFORMAT_YUV420; q->param.mfx.FrameInfo.BitDepthLuma = desc->comp[0].depth; q->param.mfx.FrameInfo.BitDepthChroma = desc->comp[0].depth; q->param.mfx.FrameInfo.Shift = desc->comp[0].depth > 8; +// TODO: detect version of MFX--if the minor version is greater than +// or equal to 19, then can use the same alignment settings as H.264 +// for HEVC +q->width_align = avctx->codec_id == AV_CODEC_ID_HEVC ? 32 : 16; +q->param.mfx.FrameInfo.Width = FFALIGN(avctx->width, q->width_align); + +if (avctx->flags & AV_CODEC_FLAG_INTERLACED_DCT) { +// it is important that PicStruct be setup correctly from the +// start--otherwise, encoding doesn't work and results in a bunch +// of incompatible video parameter errors +q->param.mfx.FrameInfo.PicStruct = MFX_PICSTRUCT_FIELD_TFF; +// height alignment always must be 32 for interlaced video +q->height_align = 32; +} else { +q->param.mfx.FrameInfo.PicStruct = MFX_PICSTRUCT_PROGRESSIVE; +// for progressive video, the height should be aligned to 16 for +// H.264. For HEVC, depending on the version of MFX, it should be +// either 32 or 16. The lower number is better if possible. +q->height_align = avctx->codec_id == AV_CODEC_ID_HEVC ? 32 : 16; +} +q->param.mfx.FrameInfo.Height = FFALIGN(avctx->height, q->height_align); + if (avctx->hw_frames_ctx) { AVHWFramesContext *frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data; AVQSVFramesContext *frames_hwctx = frames_ctx->hwctx; @@ -898,7 +915,7 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame, } else { /* make a copy if the input is not padded as libmfx requires */ if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) { -qf->frame->height = FFALIGN(frame->height, 32); +qf->frame->height = FFALIGN(frame->height, q->height_align); qf->frame->width = FFALIGN(frame->width, q->width_align); ret = ff_get_buffer(q->avctx, qf->frame, AV_GET_BUFFER_FLAG_REF); diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h index 361d9333d8..12e3444b75 100644 --- a/libavcodec/qsvenc.h +++ b/libavcodec/qsvenc.h @@ -84,6 +84,7 @@ typedef struct QSVEncContext { int packet_size; int width_align; +int height_align; mfxVideoParam param; mfxFrameAllocRequest req; -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] qsvenc: Use MFXVideoENCODE_Query() to update the parameters
From 2882d77d76805e74fe139f6763f91c39746bebaf Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 5 May 2017 18:49:57 -0700 Subject: [PATCH 1/2] qsvenc: Use MFXVideoENCODE_Query() to update the parameters Purpose: Fill out the default/unset parameters with ones actually in use. Note: Matches the current MediaSDK example code. This code used to be present in ffmpeg and was eliminated in revision 1f26a23 on Oct. 31, 2016 (qsv: Merge libav implementation, at https://github.com/FFmpeg/FFmpeg/commit/1f26a231bb065276cd80ce02957c759f3197edfa#diff-7d84a34d58597bb7aa4b8239dca1f9f8). Already applied to libav. Reviewed-by: Luca Barbato <lu_z...@gentoo.org> (cherry picked from commit b22094d74901fb3ac203c8322f8d84aded470bfb) Signed-off-by: Mark Thompson <s...@jkqxz.net> Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- libavcodec/qsvenc.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index 9c385a79d8..68d4f5edd0 100644 --- a/libavcodec/qsvenc.c +++ b/libavcodec/qsvenc.c @@ -740,10 +740,18 @@ int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q) if (ret < 0) return ret; +ret = MFXVideoENCODE_Query(q->session, >param, >param); +if (ret == MFX_WRN_PARTIAL_ACCELERATION) { +av_log(avctx, AV_LOG_WARNING, "Encoder will work with partial HW acceleration\n"); +} else if (ret < 0) { +return ff_qsv_print_error(avctx, ret, + "Error querying encoder params"); +} + ret = MFXVideoENCODE_QueryIOSurf(q->session, >param, >req); if (ret < 0) return ff_qsv_print_error(avctx, ret, - "Error querying the encoding parameters"); + "Error querying (IOSurf) the encoding parameters"); if (opaque_alloc) { ret = qsv_init_opaque_alloc(avctx, q); -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixed problems with QuickSync (QSV) interlaced video encoding
Please disregard this patch--I'm submitting two new patches that have already been reviewed and applied to libav. Aaron Levinson On 4/13/2017 11:36 PM, Aaron Levinson wrote: From da3899b24ad89b4788a3b8191d53b26f5eec328e Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Thu, 13 Apr 2017 23:12:30 -0700 Subject: [PATCH] Fixed problems with QuickSync (QSV) interlaced video encoding Purpose: Fixed problems with QuickSync (QSV) interlaced video encoding that were introduced in revision 1f26a23 on Oct. 31, 2016 (qsv: Merge libav implementation, at https://github.com/FFmpeg/FFmpeg/commit/1f26a231bb065276cd80ce02957c759f3197edfa#diff-7d84a34d58597bb7aa4b8239dca1f9f8). As a result of the qsv libav merge, when attempting to encode interlaced video, it doesn't work and instead results in a bunch of incompatible video parameter errors. Comments: -- qsvenc.c / .h: a) Added code back in to set PicStruct appropriately based on whether or not interlaced or progressive video is being encoded. Also reintroduced the related code to set the height alignment. The height alignment code was also enhanced slightly (compared to the version in 3.2.4) to properly handle progressive video when the HEVC encoder is used. The elimination of this code is the main reason why interlaced video encoding stopped working. b) Reintroduced code to call MFXVideoENCODE_Query() after calling init_video_param(). This isn't strictly required to fix the interlaced video encoding issue, but it represents a generally good practice to make sure that one is working with the right parameter values. --- libavcodec/qsvenc.c | 33 + libavcodec/qsvenc.h | 1 + 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index 9c385a7..24ac390 100644 --- a/libavcodec/qsvenc.c +++ b/libavcodec/qsvenc.c @@ -358,6 +358,9 @@ static int init_video_param(AVCodecContext *avctx, QSVEncContext *q) return AVERROR_BUG; q->param.mfx.CodecId = ret; +// TODO: detect version of MFX--if the minor version is greater than +// or equal to 19, then can use the same alignment settings as H.264 +// for HEVC q->width_align = avctx->codec_id == AV_CODEC_ID_HEVC ? 32 : 16; if (avctx->level > 0) @@ -381,20 +384,34 @@ static int init_video_param(AVCodecContext *avctx, QSVEncContext *q) ff_qsv_map_pixfmt(sw_format, >param.mfx.FrameInfo.FourCC); -q->param.mfx.FrameInfo.Width = FFALIGN(avctx->width, q->width_align); -q->param.mfx.FrameInfo.Height = FFALIGN(avctx->height, 32); q->param.mfx.FrameInfo.CropX = 0; q->param.mfx.FrameInfo.CropY = 0; q->param.mfx.FrameInfo.CropW = avctx->width; q->param.mfx.FrameInfo.CropH = avctx->height; q->param.mfx.FrameInfo.AspectRatioW = avctx->sample_aspect_ratio.num; q->param.mfx.FrameInfo.AspectRatioH = avctx->sample_aspect_ratio.den; -q->param.mfx.FrameInfo.PicStruct = MFX_PICSTRUCT_PROGRESSIVE; q->param.mfx.FrameInfo.ChromaFormat = MFX_CHROMAFORMAT_YUV420; q->param.mfx.FrameInfo.BitDepthLuma = desc->comp[0].depth; q->param.mfx.FrameInfo.BitDepthChroma = desc->comp[0].depth; q->param.mfx.FrameInfo.Shift = desc->comp[0].depth > 8; +q->param.mfx.FrameInfo.Width = FFALIGN(avctx->width, q->width_align); +if (avctx->flags & AV_CODEC_FLAG_INTERLACED_DCT) { +// it is important that PicStruct be setup correctly from the +// start--otherwise, encoding doesn't work and results in a bunch +// of incompatible video parameter errors +q->param.mfx.FrameInfo.PicStruct = MFX_PICSTRUCT_FIELD_TFF; +// height alignment always must be 32 for interlaced video +q->height_align = 32; +} else { +q->param.mfx.FrameInfo.PicStruct = MFX_PICSTRUCT_PROGRESSIVE; +// for progressive video, the height should be aligned to 16 for +// H.264. For HEVC, depending on the version of MFX, it should be +// either 32 or 16. The lower number is better if possible. +q->height_align = avctx->codec_id == AV_CODEC_ID_HEVC ? 32 : 16; +} +q->param.mfx.FrameInfo.Height = FFALIGN(avctx->height, q->height_align); + if (avctx->hw_frames_ctx) { AVHWFramesContext *frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data; AVQSVFramesContext *frames_hwctx = frames_ctx->hwctx; @@ -740,10 +757,18 @@ int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q) if (ret < 0) return ret; +ret = MFXVideoENCODE_Query(q->session, >param, >param); +if (ret == MFX_WRN_PARTIAL_ACCELERATION) { +av_log(avctx, AV_LOG_WARNING, "Encoder will work with part
Re: [FFmpeg-devel] [PATCH] build: remove --enable-raise-major configure option
On 5/6/2017 2:59 PM, James Almer wrote: It's not used by anything, has dubious usefulness, the reasons for which it was introduced are no longer valid, and only serves to add complexity to the build system. Signed-off-by: James Almer <jamr...@gmail.com> --- Makefile | 6 -- configure | 2 -- ffbuild/library.mak | 2 +- ffbuild/libversion.sh | 2 -- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/Makefile b/Makefile index d414cf841e..d177311262 100644 --- a/Makefile +++ b/Makefile @@ -107,12 +107,6 @@ $(1) := $(1)-yes := endef -ifdef CONFIG_RAISE_MAJOR -RAISE_MAJOR = 100 -else -RAISE_MAJOR = 0 -endif - define DOSUBDIR $(foreach V,$(SUBDIR_VARS),$(eval $(call RESET,$(V SUBDIR := $(1)/ diff --git a/configure b/configure index b76f9ce567..e28f27a739 100755 --- a/configure +++ b/configure @@ -109,7 +109,6 @@ Configuration options: --enable-grayenable full grayscale support (slower color) --disable-swscale-alpha disable alpha channel support in swscale --disable-alldisable building components, libraries and programs - --enable-raise-major increase major version numbers in sonames [no] Program options: --disable-programs do not build command line programs @@ -1686,7 +1685,6 @@ CONFIG_LIST=" neon_clobber_test ossfuzz pic -raise_major thumb valgrind_backtrace xmm_clobber_test diff --git a/ffbuild/library.mak b/ffbuild/library.mak index cfc2d36067..22f1e4c37f 100644 --- a/ffbuild/library.mak +++ b/ffbuild/library.mak @@ -34,7 +34,7 @@ $(TESTPROGS) $(TOOLS): %$(EXESUF): %.o $$(LD) $(LDFLAGS) $(LDEXEFLAGS) $$(LD_O) $$(filter %.o,$$^) $$(THISLIB) $(FFEXTRALIBS) $$(ELIBS) $(SUBDIR)lib$(NAME).version: $(SUBDIR)version.h | $(SUBDIR) - $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< $(RAISE_MAJOR) > $$@ + $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< > $$@ $(SUBDIR)lib$(FULLNAME).pc: $(SUBDIR)version.h | $(SUBDIR) $$(M) $$(SRC_PATH)/ffbuild/pkgconfig_generate.sh $(NAME) "$(DESC)" diff --git a/ffbuild/libversion.sh b/ffbuild/libversion.sh index 687adf28bc..990ce9f640 100755 --- a/ffbuild/libversion.sh +++ b/ffbuild/libversion.sh @@ -5,10 +5,8 @@ toupper(){ name=lib$1 ucname=$(toupper ${name}) file=$2 -raise_major=$3 eval $(awk "/#define ${ucname}_VERSION_M/ { print \$2 \"=\" \$3 }" "$file") -eval ${ucname}_VERSION_MAJOR=$((${ucname}_VERSION_MAJOR+${raise_major})) eval ${ucname}_VERSION=\$${ucname}_VERSION_MAJOR.\$${ucname}_VERSION_MINOR.\$${ucname}_VERSION_MICRO eval echo "${name}_VERSION=\$${ucname}_VERSION" eval echo "${name}_VERSION_MAJOR=\$${ucname}_VERSION_MAJOR" LGTM. However, it seems that some documentation needs to be updated as well. In doc/developer.texi, it states: "Incrementing the third component means a noteworthy binary compatible change (e.g. encoder bug fix that matters for the decoder). The third component always starts at 100 to distinguish FFmpeg from Libav." Note that my review only covers the content of the patch, and I don't know whether or not it makes sense to discontinue this practice. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video
On 5/5/2017 2:26 AM, wm4 wrote: On Fri, 5 May 2017 01:11:17 -0700 Aaron Levinson <alevi...@aracnet.com> wrote: As I said on IRC, I'm skeptic against this, but I'm also not sure whether I understand the situation. First, your change seems a bit fragile, and almost relies on coincidences. This could easily break in the future. (And if we add a FATE test, it would probably hit random people trying to change ffmpeg.c, and would cause them more work.) I guess I don't understand why you think this change is fragile, relies on coincidences, and could easily break in the future. Well, the implemented and working logic is that ffmpeg.c waits until it has encoded packets for every stream, and then writes the file headers. That should work pretty well to let metadata naturally "flow" through decoder, filters, and encoder. But your case doesn't fit right into it, so you apparently have to make it somehow call do_video_out() more. (Also a thing I don't understand - do_video_out() should have been called at least once to encode a video packet.) That's not how ffmpeg.c behaves today. It writes the file headers before it has any encoded packets for the _last_ output stream that is initialized. With my change, I make sure that the file headers aren't written until after it has had an opportunity to generate encoded packets for at least one input frame for all output streams, although there are some FATE test cases in which input frames aren't ready yet by this point, which explains why the existence of the fallback in my patch is necessary. It doesn't call do_video_out() any more times with my patch than prior to my patch. It just changes the order of when check_init_output_file() is called with respect to do_video_out() (and do_audio_out()). And looking at how field_order is set, the existing code doesn't make sense in the first place. So maybe it would be better to fix the actual problem, instead of adding another workaround. At least that's my thinking. The existing code is already fragile. Indeed. That (and me being sleep deprived) probably contributes to that I just don't understand your fix. Also feel free to ignore me - I'm neither maintainer of ffmpeg.c nor an authority on it. As I indicated in previous e-mails on this topic, the behavior of the current code can change depending on timing when there is more than one stream. This patch should make things more deterministic. And, I do think it is appropriate to add an interlaced video decoding FATE test, although there is no point to doing so until this bug is fixed, as the current behavior is broken. Looking at the current do_video_out() function (which btw. is way too long and also has broken indentation), I see the following line: mux_par->field_order = AV_FIELD_PROGRESSIVE; (and similar assignments to field_order to set interlaced modes) This is apparently run on every frame. This looks pretty wrong to me. It should _never_ change the codecpar after headers were written. This is simply invalid API use. If this is really a parameter that can change per frame, and that the _muxer_ needs this information (per packet I suppose), a side data signaling the field order should be added. mpegtsenc.c doesn't seem to read field_order at all, though. So I remain confused. Yes, I agree that the approach of changing field_order for every frame is not the right way to go, but this code was already existing. A future patch could undertake the task of handling interlaced video in a more proper way, and I wrote a TODO comment to that point. I don't understand that comment either. As I already said, do_video_out() has to be _necessarily_ called before the muxer can be initialized (aka writing headers). The way the code is currently structured, do_video_out() is called by reap_filters() after it calls init_output_stream(), and it is init_output_stream() that makes the call to check_init_output_file(), which is where the call to avformat_write_header() is done. reap_filters() is the only function that calls do_video_out(), and I think it is pretty clear that this will be done after it calls init_output_stream() from looking at the source code. My TODO has to do with setting up the field order based on header information, rather than waiting till it gets the first input frame. That's the theory at least, but I don't currently know how to do that in practice within ffmpeg.c. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Added require fallback for libmfx in the case that pkg-config cannot find libmfx
On 5/5/2017 4:50 AM, Michael Niedermayer wrote: > On Fri, May 05, 2017 at 11:36:05AM +0200, Hendrik Leppkes wrote: >> On Fri, May 5, 2017 at 9:57 AM, Clément Bœsch <u...@pkh.me> wrote: >>> On Fri, May 05, 2017 at 12:54:12AM -0700, Aaron Levinson wrote: >>> [...] >>>>> Back to your issue: you should fix the .pc in the upstream project, this >>>>> is the correct fix. >>>> >>>> The "upstream project" in this case is the Intel Media SDK. This is not an >>>> open source project, and developers get it through Intel in the form of an >>>> installation package on Windows. So, there is no opportunity to "fix the >>>> .pc"--there isn't one, nor is there any way to contribute one. >>> >>> OK so we have a common configure flag for supporting two different >>> projects with different authors, licensing, etc? >>> >>> Maybe we should have 2 configure flags? Add --enable-libmfxintel or >>> whatever? >>> >> >> I think having separate flags would just be annoying. Lucas >> redistribution is in no sense "official" in any way, and what if >> someone else comes up with another re-package of the same thing? > >> The key information here is, upstream libmfx does not actually have a >> pkg-config file, that some re-distributions add one is nice and all, >> and we can use that if present, but we should also be able to use the >> upstream variant which does not come with one at all. > > +1 Per some discussion on IRC, I've created a new patch that includes a detailed comment in configure for why require is being used. I also altered the text for the patch. Thanks, Aaron Levinson --- From 5d8fb32801accc06655c4fae700652958bc4350e Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 5 May 2017 18:16:03 -0700 Subject: [PATCH] configure: Added require alternative for libmfx to support alternate installation options Purpose: Added require alternative for libmfx in the case that pkg-config cannot find libmfx. On Linux, most people likely get libmfx via https://github.com/lu-zero/mfx_dispatch , but on Windows, the most well-known way to get libmfx is via the Intel Media SDK, which provides a static build of libmfx.lib and also provides the source code for building libmfx yourself. If built this way, there are no pkg-config files to be found. Comments: -- configure: Altered enabled libmfx step to use use_pkg_config() instead of require_pkg_config(), and, if use_pkg_config() fails, it falls back to require(). Also added explanatory comment. Note that the reason that require() is passed -llibmfx as the last argument, instead of -lmfx, is the file name for the library produced from the Intel Media SDK starts with "libmfx". Apparently, the filename for the library produced via https://github.com/lu-zero/mfx_dispatch starts with "mfx". Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- configure | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/configure b/configure index a008b41473..296bff9dc7 100755 --- a/configure +++ b/configure @@ -5788,7 +5788,14 @@ enabled libgsm&& { for gsm_hdr in "gsm.h" "gsm/gsm.h"; do done || die "ERROR: libgsm not found"; } enabled libilbc && require libilbc ilbc.h WebRtcIlbcfix_InitDecode -lilbc enabled libkvazaar&& require_pkg_config "kvazaar >= 0.8.1" kvazaar.h kvz_api_get -enabled libmfx&& require_pkg_config libmfx "mfx/mfxvideo.h" MFXInit +# While it may appear that require is being used as a pkg-config +# fallback for libmfx, it is actually being used to detect a different +# installation route altogether. If libmfx is installed via the Intel +# Media SDK or Intel Media Server Studio, these don't come with +# pkg-config support. Instead, users should make sure that the build +# can find the libraries and headers through other means. +enabled libmfx&& { use_pkg_config libmfx "mfx/mfxvideo.h" MFXInit || + { require libmfx "mfx/mfxvideo.h" MFXInit -llibmfx && warn "using libmfx without pkg-config"; } } enabled libmodplug&& require_pkg_config libmodplug libmodplug/modplug.h ModPlug_Load enabled libmp3lame&& require "libmp3lame >= 3.98.3" lame/lame.h lame_set_VBR_quality -lmp3lame enabled libnut&& require libnut libnut.h nut_demuxer_init -lnut -- 2.12.2.windows.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Made minor changes to get the decklink avdevice code to build using Visual C++
On 5/5/2017 4:57 PM, Marton Balint wrote: > > > On Fri, 5 May 2017, Aaron Levinson wrote: > >> On 4/16/2017 4:11 PM, Aaron Levinson wrote: >>> On 4/15/2017 6:13 AM, Aaron Levinson wrote: >>>> On 4/15/2017 4:19 AM, Marton Balint wrote: >>>>> >>>>> On Thu, 13 Apr 2017, Aaron Levinson wrote: >>>>> >>>>>> On 4/13/2017 1:23 PM, Hendrik Leppkes wrote: >>>>> [...] >> >> Ping, and in addition, I've provided a new patch below since the >> original won't apply anymore. I've also adjusted the patch text a >> little bit. The original patch went through a few different reviews. >> These changes are necessary to get the decklink code to build using >> MSVC on Windows. >> >> Thanks, >> Aaron Levinson >> >> >> [...] >> case PTS_SRC_WALLCLOCK: >> -pts = av_rescale_q(wallclock, AV_TIME_BASE_Q, time_base); >> +{ >> +/* MSVC does not support compound literals like >> AV_TIME_BASE_Q >> + * in C++ code (compiler error C4576) */ >> +// pts = av_rescale_q(wallclock, AV_TIME_BASE_Q, time_base); > > I'd rather remove the old code, i see no point in keeping it as a comment. Done--new patch with this change follows at the end. >> [...] > > The decklink part seems fine otherwise, maybe you should get an ACK from > Hendrik for the configure part, which I have no opinion about. Hendrik already reviewed the configure changes last month, and the configure part of the patch went through three iterations till it was determined that it was adequate, although it wouldn't hurt to have another look since then. New patch with the slight alteration follows. Thanks, Aaron Levinson -- From 269f836b50f8949dbc45a2578bd16a53952b Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 5 May 2017 17:59:21 -0700 Subject: [PATCH] decklink: Fixed MSVC build issues Purpose: Made minor changes to get the decklink avdevice code to build using Visual C++. Notes: Made changes to configure per Hendrik Leppkes's review of first and second versions of patch. Also made slight alterations per Marton Balint's reviews. Comments: -- configure: Added if enabled decklink section and setting decklink_indev_extralibs and decklink_outdev_extralibs here for both mingw and Windows. Also eliminated the setting of these variables in the mingw section earlier in the file. -- libavdevice/decklink_common.cpp: Switched the order of the include of libavformat/internal.h to workaround build issues with Visual C++. See comment in file for more details. -- libavdevice/decklink_dec.cpp: a) Rearranged the include of libavformat/internal.h (for reasons as described above). b) Made slight alteration to an argument for call to av_rescale_q() to workaround a compiler error with Visual C++. This appears to only be an issue when building C++ files with Visual C++. See comment in code for more details. -- libavdevice/decklink_enc.cpp: Rearranged the include of libavformat/internal.h (for reasons as described above). Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- configure | 11 +-- libavdevice/decklink_common.cpp | 7 ++- libavdevice/decklink_dec.cpp| 16 ++-- libavdevice/decklink_enc.cpp| 7 ++- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/configure b/configure index 2e77a1034e..6f2c900a7b 100755 --- a/configure +++ b/configure @@ -4850,8 +4850,6 @@ case $target_os in else target_os=mingw32 fi -decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" -decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" LIBTARGET=i386 if enabled x86_64; then LIBTARGET="i386:x86-64" @@ -5957,6 +5955,15 @@ if ! disabled sdl2; then fi enabled sdl2 && enable sdl && add_cflags $sdl2_cflags && add_extralibs $sdl2_extralibs +if enabled decklink; then +case $target_os in +mingw32*|mingw64*|win32|win64) +decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" +decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" +;; +esac +fi + disabled securetransport || { check_func SecIdentityCreate "-Wl,-framework,CoreFoundation -Wl,-framework,Security" && check_lib securetransport "Security/SecureTransport.h Security/Security.h" "SSLCreateCont
Re: [FFmpeg-devel] [PATCH v2 0/9] Removing HEVCContext depencencies
The entire patch set LGTM. Aaron Levinson On 5/3/2017 6:41 PM, James Almer wrote: On 5/3/2017 9:39 PM, Aaron Levinson wrote: James, Can you document which patches depend on other patches in this patch set to make it easier to review this patch set in chunks? Patch 2 depends on 1. Patches 3, 4 and 5 are functionally standalone but need previous patches to apply cleanly. Patches 7 and 6 depend on every patch before themselves. Patch 8 depends on patches 1 and 2 (and maybe others to apply cleanly). Thanks, Aaron Levinson On 5/2/2017 2:55 PM, James Almer wrote: Some changes and fixes suggested on IRC. Some patches are v3 as they have been sent before outside the previous set. James Almer (9): hevcdec: remove HEVCContext usage from hevc_sei hevcdec: move SEI message parsing into a separate header hevcdec: remove HEVCContext usage from ff_hevc_compute_poc() hevcdec: move SliceHeader struct definition to hevc_ps hevc_parser: use ff_h2645_packet_split() to parse NAL units hevc_parser: remove HEVCContext usage hevc_parser: move slice header parsing to its own function hevc_parse: decode SEI message NALUs in extradata doc/libav_merge: remove line about ADVANCED_PARSER doc/libav-merge.txt| 1 - libavcodec/Makefile| 4 +- libavcodec/hevc_parse.c| 21 ++- libavcodec/hevc_parse.h| 7 +- libavcodec/hevc_parser.c | 415 - libavcodec/hevc_ps.c | 23 +++ libavcodec/hevc_ps.h | 88 ++ libavcodec/hevc_refs.c | 27 +-- libavcodec/hevc_sei.c | 193 + libavcodec/hevc_sei.h | 125 ++ libavcodec/hevcdec.c | 94 +- libavcodec/hevcdec.h | 137 +-- libavcodec/mediacodecdec.c | 4 +- 13 files changed, 532 insertions(+), 607 deletions(-) create mode 100644 libavcodec/hevc_sei.h ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Added require fallback for libmfx in the case that pkg-config cannot find libmfx
On 5/5/2017 12:57 AM, Clément Bœsch wrote: On Fri, May 05, 2017 at 12:54:12AM -0700, Aaron Levinson wrote: [...] Back to your issue: you should fix the .pc in the upstream project, this is the correct fix. The "upstream project" in this case is the Intel Media SDK. This is not an open source project, and developers get it through Intel in the form of an installation package on Windows. So, there is no opportunity to "fix the .pc"--there isn't one, nor is there any way to contribute one. OK so we have a common configure flag for supporting two different projects with different authors, licensing, etc? Maybe we should have 2 configure flags? Add --enable-libmfxintel or whatever? The licensing is the same--lu_zero's (Luca Barbato's) libmfx project just packages the files from the Intel Media SDK (or more likely Intel Media Server Studio). And this means that there is effectively one author as well. While the licensing for libmfx (really, mfx_dispatch) is suitable for open source projects, I don't think the same can be claimed for the Intel Media SDK or the Intel Media Server Studio as a whole, and it is technically necessary to install one of those to get the mfx_dispatch sources. I really didn't want this to turn into another non-free discussion ala Blackmagic. I'm fairly certain that Intel's intention is that the mfx_dispatch sources are suitable for use in open source projects, but it would be helpful if Intel offered the mfx_dispatch sources independent of either the Intel Media SDK or Intel Media Server Studio. Nevertheless, it does seem reasonable to have another configure flag to enable the use of libmfx via Intel's official installers, which don't provide anything for pkg-config. Even Intel Media Server Studio, which is available for Linux, doesn't provide anything for pkg-config, and the instructions at http://www.intel.com/content/dam/www/public/us/en/documents/white-papers/quicksync-video-ffmpeg-install-valid.pdf describe a process for manually creating a libmfx.pc file in order to be able to build ffmpeg with Intel Media Server Studio. I'll scrap this patch and work on a different patch that uses this approach. Aaron ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video
On 5/5/2017 12:55 AM, wm4 wrote: On Thu, 4 May 2017 23:46:30 -0700 Aaron Levinson <alevi...@aracnet.com> wrote: On 4/12/2017 6:08 PM, Aaron Levinson wrote: On 3/26/2017 10:34 AM, Aaron Levinson wrote: On 3/26/2017 4:41 AM, Matthias Hunstock wrote: Am 26.03.2017 um 11:50 schrieb Aaron Levinson: >>>>> [...] >>> [...] I've provided a new version of the patch. When I created the first version of the patch on March 26th, this was the first patch that I submitted to ffmpeg, and some aspects were rough. I had indicated that the patch passed regression tests, but all I did was run "make fate", instead of "make fate SAMPLES=fate-suite/", and once I understood that I should use fate-suite, I discovered that some of the FATE tests failed with this patch. I fixed the issues with the patch, adjusted some comments, and adjusted the patch description text. I've confirmed that this patch passes all fate-suite tests for 64-bit MSVC on Windows and 64-bit gcc on Linux. Thanks, Aaron Levinson From 85aa383f5753795652bae015f4fe91b016f7387e Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Thu, 4 May 2017 22:54:31 -0700 Subject: [PATCH] ffmpeg: Fixed bug with decoding interlaced video Purpose: Fixed bug in ffmpeg encountered when decoding interlaced video. In some cases, ffmpeg would treat it as progressive. As a result of the change, the issues with interlaced video are fixed. Detailed description of problem: Run the following command: "ffmpeg -i test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, test8_1080i.h264 is an H.264-encoded file with 1080i59.94 (interlaced). Prior to the patch, the following output is generated: Input #0, h264, from 'test8_1080i.h264': Duration: N/A, bitrate: N/A Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc Stream mapping: Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) Press [q] to stop, [?] for help Output #0, mpegts, to 'test8_1080i_mp2_2.ts': Metadata: encoder : Lavf57.72.100 Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc Metadata: encoder : Lavc57.92.100 mpeg2video which demonstrates the bug. The output should instead look like: Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc The bug is caused by the fact that reap_filters() calls init_output_stream(), which in turn calls check_init_output_file(), and this is all done prior to the first call to do_video_out(). An initial call to do_video_out() is necessary to populate the interlaced video information to the output stream's codecpar (mux_par->field_order in do_video_out()). However, check_init_output_file() calls avformat_write_header() prior to the initial call to do_video_out(), so field_order is populated too late, and the header is written with the default field_order value, progressive. Comments: -- ffmpeg.c: a) Enhanced init_output_stream() to take an additional input indicating whether or not check_init_output_file() should be called. There are other places that call init_output_stream(), and it was important to make sure that these continued to work properly. b) Adjusted reap_filters() such that, in the case that init_output_stream() is called, it indicates that it should not call check_init_output_file() in init_output_stream(). Instead, it makes the call to check_init_output_file() directly, but after it does the filtered frame setup and processing. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- ffmpeg.c | 61 + 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index e798d92277..45dbfafc04 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -1400,7 +1400,7 @@ static void do_video_stats(OutputStream *ost, int frame_size) } } -static int init_output_stream(OutputStream *ost, char *error, int error_len); +static int init_output_stream(OutputStream *ost, char *error, int error_len, int do_check_output_file); static void finish_output_stream(OutputStream *ost) { @@ -1415,6 +1415,8 @@ static void finish_output_stream(OutputStream *ost) } } +static int check_init_output_file(OutputFile *of, int file_index); + /** * Get and encode new output from any of the filtergraphs, without causing * activity. @@ -1433,6 +1435,7 @@ static int reap_filters(int flush) AVFilterContext *filter; AVCodecContext *enc = ost->enc_ctx; int ret = 0; +int did_init_output_stream = 0; if (!ost->filter || !ost->filter->g
Re: [FFmpeg-devel] [RFC]lavu/timecode: Increase AV_TIMECODE_STR_SIZE.
On 5/5/2017 12:44 AM, Carl Eugen Hoyos wrote: 2017-05-05 9:42 GMT+02:00 Aaron Levinson <alevi...@aracnet.com>: On 5/5/2017 12:20 AM, Carl Eugen Hoyos wrote: 2017-05-05 3:29 GMT+02:00 Aaron Levinson <alevi...@aracnet.com>: On 5/4/2017 4:27 PM, Carl Eugen Hoyos wrote: Hi! Attached patch is one possibility to fix the following warning with gcc 7: libavutil/timecode.c: In function ‘av_timecode_make_string’: libavutil/timecode.c:103:60: warning: ‘%02d’ directive output may be truncated writing between 2 and 10 bytes into a region of size between 0 and 7 [-Wformat-truncation=] snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^~~~ libavutil/timecode.c:103:41: note: directive argument in the range [0, 2147483647] snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^~~~ libavutil/timecode.c:103:5: note: ‘snprintf’ output between 12 and 32 bytes into a destination of size 16 snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^ neg ? "-" : "", ~~~ hh, mm, ss, drop ? ';' : ':', ff); ~ Several similar warnings are printed, an alternative is to disable the warning. The warning seemed wrong on first sight but may actually be correct, we accept invalid fps for timecode. Regarding the change in your patch: -#define AV_TIMECODE_STR_SIZE 16 +#define AV_TIMECODE_STR_SIZE 23 It seems like 23 characters wouldn't be sufficient based on the warning: "output between 12 and 32 bytes into a destination of size 16". I would guess that you would need at least 32 characters and perhaps one more (?) for the terminating null character to avoid that warning. How can I reproduce / test your findings? #define AV_TIMECODE_STR_SIZE 32 or 33 and confirm that gcc 7 warnings go away. Of course! Since the warnings go away with 23, they also go away with 32. I just wonder why 23 isn't enough in your opinion? You hadn't previously indicated that the warnings disappeared when 23 is used, so I was operating based on the warning text, which said that the output text could consume as much as 32 bytes. It seems odd that the warning text would disappear in this case, but if that's sufficient, it seems okay to apply. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Added require fallback for libmfx in the case that pkg-config cannot find libmfx
On 5/5/2017 12:42 AM, Clément Bœsch wrote: On Fri, May 05, 2017 at 12:11:27AM -0700, Aaron Levinson wrote: On 4/19/2017 10:43 AM, Aaron Levinson wrote: On 4/14/2017 6:51 PM, Aaron Levinson wrote: From e0c73c054add0137901d0bf7a7893e42e7e566c8 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 14 Apr 2017 18:38:37 -0700 Subject: [PATCH] Added require fallback for libmfx in the case that pkg-config cannot find libmfx Purpose: Added require fallback for libmfx in the case that pkg-config cannot find libmfx. On Linux, most people likely get libmfx via https://github.com/lu-zero/mfx_dispatch , but on Windows, the most well-known way to get libmfx is via the Intel Media SDK, which provides a static build of libmfx.lib and also provides the source code for building libmfx yourself. If built this way, there are no pkg-config files to be found. The changes utilize a similar approach to that already done for libx264 in configure. Comments: -- configure: Altered enabled libmfx step to use use_pkg_config() instead of require_pkg_config(), and, if use_pkg_config() fails, it falls back to require(). Note that the reason that require() is passed -llibmfx as the last argument, instead of -lmfx, is the file name for the library produced from the Intel Media SDK starts with "libmfx". Apparently, the filename for the library produced via https://github.com/lu-zero/mfx_dispatch starts with "mfx". --- configure | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 3bea057..b20a0b4 100755 --- a/configure +++ b/configure @@ -5819,7 +5819,8 @@ enabled libgsm&& { for gsm_hdr in "gsm.h" "gsm/gsm.h"; do done || die "ERROR: libgsm not found"; } enabled libilbc && require libilbc ilbc.h WebRtcIlbcfix_InitDecode -lilbc enabled libkvazaar&& require_pkg_config "kvazaar >= 0.8.1" kvazaar.h kvz_api_get -enabled libmfx&& require_pkg_config libmfx "mfx/mfxvideo.h" MFXInit +enabled libmfx&& { use_pkg_config libmfx "mfx/mfxvideo.h" MFXInit || + { require libmfx "mfx/mfxvideo.h" MFXInit -llibmfx && warn "using libmfx without pkg-config"; } } enabled libmodplug&& require_pkg_config libmodplug libmodplug/modplug.h ModPlug_Load enabled libmp3lame&& require "libmp3lame >= 3.98.3" lame/lame.h lame_set_VBR_quality -lmp3lame enabled libnut&& require libnut libnut.h nut_demuxer_init -lnut Pinging this patch submission again. Thanks, Aaron Levinson And again. This patch is pretty straightforward, and considering that this approach was deemed suitable for libx264 It wasn't, Carl Eugen insisted in doing this against everyone opinion, but we tolerated his whim because he didn't want to use or install a 80kB package installed on virtually every system that supports x264. Anyway, here is a nasty side effect of doing this: you use PKG_CONFIG_PATH to make sure it's linking against a specific local build of your lib, but you did it wrong and it ends up linking silently against the system one instead of failing like you would like to. We predicted that allowing this hack for x264 was going to bring up patches like this and we were right. So maybe we should actually drop the hack for libx264. Back to your issue: you should fix the .pc in the upstream project, this is the correct fix. The "upstream project" in this case is the Intel Media SDK. This is not an open source project, and developers get it through Intel in the form of an installation package on Windows. So, there is no opportunity to "fix the .pc"--there isn't one, nor is there any way to contribute one. The https://github.com/lu-zero/mfx_dispatch project is not maintained by Intel, and I would argue that it is preferable to obtain the relevant files via the official Intel release. This is not the same situation as exists with libx264, which is an open source project. This is not much different in concept than the approach someone needs to take to get the Blackmagic header files, which are available via the Blackmagic SDK, and I don't see it using pkg-config for Blackmagic. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC]lavu/timecode: Increase AV_TIMECODE_STR_SIZE.
On 5/5/2017 12:20 AM, Carl Eugen Hoyos wrote: 2017-05-05 3:29 GMT+02:00 Aaron Levinson <alevi...@aracnet.com>: On 5/4/2017 4:27 PM, Carl Eugen Hoyos wrote: Hi! Attached patch is one possibility to fix the following warning with gcc 7: libavutil/timecode.c: In function ‘av_timecode_make_string’: libavutil/timecode.c:103:60: warning: ‘%02d’ directive output may be truncated writing between 2 and 10 bytes into a region of size between 0 and 7 [-Wformat-truncation=] snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^~~~ libavutil/timecode.c:103:41: note: directive argument in the range [0, 2147483647] snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^~~~ libavutil/timecode.c:103:5: note: ‘snprintf’ output between 12 and 32 bytes into a destination of size 16 snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^ neg ? "-" : "", ~~~ hh, mm, ss, drop ? ';' : ':', ff); ~ Several similar warnings are printed, an alternative is to disable the warning. The warning seemed wrong on first sight but may actually be correct, we accept invalid fps for timecode. Regarding the change in your patch: -#define AV_TIMECODE_STR_SIZE 16 +#define AV_TIMECODE_STR_SIZE 23 It seems like 23 characters wouldn't be sufficient based on the warning: "output between 12 and 32 bytes into a destination of size 16". I would guess that you would need at least 32 characters and perhaps one more (?) for the terminating null character to avoid that warning. How can I reproduce / test your findings? #define AV_TIMECODE_STR_SIZE 32 or 33 and confirm that gcc 7 warnings go away. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Made minor changes to get the decklink avdevice code to build using Visual C++
On 4/16/2017 4:11 PM, Aaron Levinson wrote: > On 4/15/2017 6:13 AM, Aaron Levinson wrote: >> On 4/15/2017 4:19 AM, Marton Balint wrote: >>> >>> On Thu, 13 Apr 2017, Aaron Levinson wrote: >>> >>>> On 4/13/2017 1:23 PM, Hendrik Leppkes wrote: >>> [...] Ping, and in addition, I've provided a new patch below since the original won't apply anymore. I've also adjusted the patch text a little bit. The original patch went through a few different reviews. These changes are necessary to get the decklink code to build using MSVC on Windows. Thanks, Aaron Levinson From 90d20d87401584138f6bdea5567b92e7dfe07f1f Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 5 May 2017 00:20:56 -0700 Subject: [PATCH] decklink: Fixed MSVC build issues Purpose: Made minor changes to get the decklink avdevice code to build using Visual C++. Notes: Made changes to configure per Hendrik Leppkes's review of first and second versions of patch. Comments: -- configure: Added if enabled decklink section and setting decklink_indev_extralibs and decklink_outdev_extralibs here for both mingw and Windows. Also eliminated the setting of these variables in the mingw section earlier in the file. -- libavdevice/decklink_common.cpp: Switched the order of the include of libavformat/internal.h to workaround build issues with Visual C++. See comment in file for more details. -- libavdevice/decklink_dec.cpp: a) Rearranged the include of libavformat/internal.h (for reasons as described above). b) Made slight alteration to an argument for call to av_rescale_q() to workaround a compiler error with Visual C++. This appears to only be an issue when building C++ files with Visual C++. See comment in code for more details. -- libavdevice/decklink_enc.cpp: Rearranged the include of libavformat/internal.h (for reasons as described above). Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- configure | 11 +-- libavdevice/decklink_common.cpp | 7 ++- libavdevice/decklink_dec.cpp| 17 +++-- libavdevice/decklink_enc.cpp| 7 ++- 4 files changed, 36 insertions(+), 6 deletions(-) diff --git a/configure b/configure index 883ec390d6..bbb9fab738 100755 --- a/configure +++ b/configure @@ -4847,8 +4847,6 @@ case $target_os in else target_os=mingw32 fi -decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" -decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" LIBTARGET=i386 if enabled x86_64; then LIBTARGET="i386:x86-64" @@ -5947,6 +5945,15 @@ if ! disabled sdl2; then fi enabled sdl2 && enable sdl && add_cflags $sdl2_cflags && add_extralibs $sdl2_extralibs +if enabled decklink; then +case $target_os in +mingw32*|mingw64*|win32|win64) +decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" +decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" +;; +esac +fi + disabled securetransport || { check_func SecIdentityCreate "-Wl,-framework,CoreFoundation -Wl,-framework,Security" && check_lib securetransport "Security/SecureTransport.h Security/Security.h" "SSLCreateContext SecItemImport" "-Wl,-framework,CoreFoundation -Wl,-framework,Security"; } diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp index f01fba953e..cbb591ce64 100644 --- a/libavdevice/decklink_common.cpp +++ b/libavdevice/decklink_common.cpp @@ -19,6 +19,12 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +/* Include internal.h first to avoid conflict between winsock.h (used by + * DeckLink headers) and winsock2.h (used by libavformat) in MSVC++ builds */ +extern "C" { +#include "libavformat/internal.h" +} + #include #ifdef _WIN32 #include @@ -28,7 +34,6 @@ extern "C" { #include "libavformat/avformat.h" -#include "libavformat/internal.h" #include "libavutil/imgutils.h" #include "libavutil/intreadwrite.h" #include "libavutil/bswap.h" diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp index 67eaf97e89..69f790d606 100644 --- a/libavdevice/decklink_dec.cpp +++ b/libavdevice/decklink_dec.cpp @@ -19,12 +19,17 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +/* Include internal.h first to avoid conflict between winsock.h (used by + * DeckLink headers) and winsock2.h (used by libavformat) in MSVC++ builds */ +extern "C" { +#include "lib
Re: [FFmpeg-devel] Added require fallback for libmfx in the case that pkg-config cannot find libmfx
On 4/19/2017 10:43 AM, Aaron Levinson wrote: > On 4/14/2017 6:51 PM, Aaron Levinson wrote: >> From e0c73c054add0137901d0bf7a7893e42e7e566c8 Mon Sep 17 00:00:00 2001 >> From: Aaron Levinson <alevi...@aracnet.com> >> Date: Fri, 14 Apr 2017 18:38:37 -0700 >> Subject: [PATCH] Added require fallback for libmfx in the case that >> pkg-config cannot find libmfx >> >> Purpose: Added require fallback for libmfx in the case that pkg-config >> cannot find libmfx. On Linux, most people likely get libmfx via >> https://github.com/lu-zero/mfx_dispatch , but on Windows, the most >> well-known way to get libmfx is via the Intel Media SDK, which >> provides a static build of libmfx.lib and also provides the source >> code for building libmfx yourself. If built this way, there are no >> pkg-config files to be found. The changes utilize a similar approach >> to that already done for libx264 in configure. >> >> Comments: >> >> -- configure: Altered enabled libmfx step to use use_pkg_config() >>instead of require_pkg_config(), and, if use_pkg_config() fails, it >>falls back to require(). Note that the reason that require() is >>passed -llibmfx as the last argument, instead of -lmfx, is the file >>name for the library produced from the Intel Media SDK starts with >>"libmfx". Apparently, the filename for the library produced via >>https://github.com/lu-zero/mfx_dispatch starts with "mfx". >> --- >> configure | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/configure b/configure >> index 3bea057..b20a0b4 100755 >> --- a/configure >> +++ b/configure >> @@ -5819,7 +5819,8 @@ enabled libgsm&& { for gsm_hdr in >> "gsm.h" "gsm/gsm.h"; do >> done || die "ERROR: libgsm not found"; } >> enabled libilbc && require libilbc ilbc.h >> WebRtcIlbcfix_InitDecode -lilbc >> enabled libkvazaar&& require_pkg_config "kvazaar >= 0.8.1" >> kvazaar.h kvz_api_get >> -enabled libmfx&& require_pkg_config libmfx >> "mfx/mfxvideo.h" MFXInit >> +enabled libmfx&& { use_pkg_config libmfx "mfx/mfxvideo.h" >> MFXInit || >> + { require libmfx "mfx/mfxvideo.h" >> MFXInit -llibmfx && warn "using libmfx without pkg-config"; } } >> enabled libmodplug&& require_pkg_config libmodplug >> libmodplug/modplug.h ModPlug_Load >> enabled libmp3lame&& require "libmp3lame >= 3.98.3" >> lame/lame.h lame_set_VBR_quality -lmp3lame >> enabled libnut&& require libnut libnut.h nut_demuxer_init >> -lnut >> > > Pinging this patch submission again. > > Thanks, > Aaron Levinson And again. This patch is pretty straightforward, and considering that this approach was deemed suitable for libx264, I don't see why there would be any issue with it being applied to libmfx as well. Here's a new version of this patch with some of the text altered slightly. Thanks, Aaron Levinson -- From aa427b09435eb99de6b308d32f066fbca71e4b18 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 5 May 2017 00:06:42 -0700 Subject: [PATCH] configure: Added require fallback for libmfx Purpose: Added require fallback for libmfx in the case that pkg-config cannot find libmfx. On Linux, most people likely get libmfx via https://github.com/lu-zero/mfx_dispatch , but on Windows, the most well-known way to get libmfx is via the Intel Media SDK, which provides a static build of libmfx.lib and also provides the source code for building libmfx yourself. If built this way, there are no pkg-config files to be found. The changes utilize a similar approach to that already done for libx264 in configure. Comments: -- configure: Altered enabled libmfx step to use use_pkg_config() instead of require_pkg_config(), and, if use_pkg_config() fails, it falls back to require(). Note that the reason that require() is passed -llibmfx as the last argument, instead of -lmfx, is the file name for the library produced from the Intel Media SDK starts with "libmfx". Apparently, the filename for the library produced via https://github.com/lu-zero/mfx_dispatch starts with "mfx". Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- configure | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/configure b/configure index c3fa9d85
Re: [FFmpeg-devel] [PATCH] Enhanced configure and Makefile to copy .pdb files to bindir for MSVC builds for make install
On 4/14/2017 6:27 PM, Aaron Levinson wrote: From 1059473c449c3079f03461bb42c2d3cc21d1b2c1 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 14 Apr 2017 18:14:21 -0700 Subject: [PATCH] Enhanced configure and Makefile to copy .pdb files to bindir for MSVC builds for make install Purpose: Enhanced configure and Makefile to copy .pdb files to bindir for MSVC builds for make install. Files are also uninstalled appropriately when make uninstall is exercised. Placing the PDB files in the same directory as other binaries can make it easier to debug, especially if the files are copied to another system. Note: General idea for how to properly handle the copying of PDB files associated with programs suggested by Hendrik Leppkes. Comments: -- configure: a) Leveraged already existing SLIB_INSTALL_EXTRA_SHLIB facility (which is already pretty specific to Windows) to add .pdb files, in addition to .lib files, for shared libraries to bindir during make install. b) Added PROG_INSTALL_EXTRA_BIN variable for MSVC builds and also added it to the section that causes it to be added to config.mak. This is used in Makefile to copy any additional files associated with programs. For MSVC, it is used to copy the pdb files associated with any executables that are built. Note that such executables are build with _g in the file name and are later copied to a filename without _g in the file name. As such, the PDB files have _g in the file name. -- Makefile: Enhanced install-progs and uninstall-progs targets to handle PROG_INSTALL_EXTRA_BIN if defined. It uses a similar procedure as already in place for SLIB_INSTALL_EXTRA_SHLIB in library.mak. --- Makefile | 2 ++ configure | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d5b51de..45c42c6 100644 --- a/Makefile +++ b/Makefile @@ -165,6 +165,7 @@ install-progs-$(CONFIG_SHARED): install-libs install-progs: install-progs-yes $(AVPROGS) $(Q)mkdir -p "$(BINDIR)" $(INSTALL) -c -m 755 $(INSTPROGS) "$(BINDIR)" + $(if $(PROG_INSTALL_EXTRA_BIN), $(INSTALL) -m 644 $(PROG_INSTALL_EXTRA_BIN) "$(BINDIR)") install-data: $(DATA_FILES) $(EXAMPLES_FILES) $(Q)mkdir -p "$(DATADIR)/examples" @@ -175,6 +176,7 @@ uninstall: uninstall-libs uninstall-headers uninstall-progs uninstall-data uninstall-progs: $(RM) $(addprefix "$(BINDIR)/", $(ALLAVPROGS)) + $(if $(PROG_INSTALL_EXTRA_BIN), $(RM) $(addprefix "$(BINDIR)/", $(PROG_INSTALL_EXTRA_BIN))) uninstall-data: $(RM) -r "$(DATADIR)" diff --git a/configure b/configure index 18d79ab..88206e3 100755 --- a/configure +++ b/configure @@ -4947,9 +4947,10 @@ case $target_os in SLIB_CREATE_DEF_CMD='$(SRC_PATH)/compat/windows/makedef $(SUBDIR)lib$(NAME).ver $(OBJS) > $$(@:$(SLIBSUF)=.def)' SLIB_INSTALL_NAME='$(SLIBNAME_WITH_MAJOR)' SLIB_INSTALL_LINKS= -SLIB_INSTALL_EXTRA_SHLIB='$(SLIBNAME:$(SLIBSUF)=.lib)' +SLIB_INSTALL_EXTRA_SHLIB='$(SLIBNAME:$(SLIBSUF)=.lib) $(SLIBNAME_WITH_MAJOR:$(SLIBSUF)=.pdb)' SLIB_INSTALL_EXTRA_LIB='$(SLIBNAME_WITH_MAJOR:$(SLIBSUF)=.def)' SHFLAGS='-dll -def:$$(@:$(SLIBSUF)=.def) -implib:$(SUBDIR)$(SLIBNAME:$(SLIBSUF)=.lib)' +PROG_INSTALL_EXTRA_BIN='$(AVPROGS-yes:%=%$(PROGSSUF)_g.pdb)' objformat="win32" ranlib=: enable dos_paths @@ -6796,6 +6797,7 @@ SLIB_INSTALL_NAME=${SLIB_INSTALL_NAME} SLIB_INSTALL_LINKS=${SLIB_INSTALL_LINKS} SLIB_INSTALL_EXTRA_LIB=${SLIB_INSTALL_EXTRA_LIB} SLIB_INSTALL_EXTRA_SHLIB=${SLIB_INSTALL_EXTRA_SHLIB} +PROG_INSTALL_EXTRA_BIN=${PROG_INSTALL_EXTRA_BIN} VERSION_SCRIPT_POSTPROCESS_CMD=${VERSION_SCRIPT_POSTPROCESS_CMD} SAMPLES:=${samples:-\$(FATE_SAMPLES)} NOREDZONE_FLAGS=$noredzone_flags Ping. Thanks, Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video
On 4/12/2017 6:08 PM, Aaron Levinson wrote: > On 3/26/2017 10:34 AM, Aaron Levinson wrote: >> On 3/26/2017 4:41 AM, Matthias Hunstock wrote: >>> Am 26.03.2017 um 11:50 schrieb Aaron Levinson: >>>> When using the following command to play back either file: >>>> ffmpeg -i -f decklink -pix_fmt uyvy422 "DeckLink SDI >>>> 4K", I noticed that with the mpegts file with the AAC audio stream, >>>> it would correctly select an interlaced video mode for the video >>>> output stream, but with the mpegts file with the Opus audio stream, >>>> it would use a progressive video mode (1080p29.97) for the video >>>> output stream. >>> >>> Which FFmpeg version did you test this with? >>> >>> There was a change related to this just short time ago. >>> >>> Does it happen with current git HEAD? >>> >>> Matthias >> >> This issue occurs with the current git HEAD. I'm aware of the >> Blackmagic improvement that was added in February to add support for >> interlaced video modes on output, and actually that's one of the reasons >> why I'm using the latest git sources, as opposed to, say, 3.2.4. This >> particular issue has nothing to do with Blackmagic, and I only used >> Blackmagic in the example that reproduces the bug because it is >> something that can be reproduced on both Windows and Linux (and >> presumably also on OS/X). The issue also occurs if I do something like >> -f rawvideo out.avi on Windows, and I'm sure that there are plenty of >> other examples. >> >> Aaron Levinson > > Has anyone had a chance to review this patch yet, which I submitted on March > 26th? To demonstrate the impact of this patch, here's some output of before > and after for an .h264 file with interlaced 1080i59.94 video content: > > Command-line: ffmpeg -i test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts > > Before patch: > > -- > > Input #0, h264, from 'test8_1080i.h264': > Duration: N/A, bitrate: N/A > Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 > DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc > Stream mapping: > Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) > Press [q] to stop, [?] for help > Output #0, mpegts, to 'test8_1080i_mp2_2.ts': > Metadata: > encoder : Lavf57.72.100 > Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR > 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc > Metadata: > encoder : Lavc57.92.100 mpeg2video > Side data: > cpb: bitrate max/min/avg: 0/0/20 buffer size: 0 vbv_delay: -1 > > -- > > After patch: > > -- > > Input #0, h264, from 'test8_1080i.h264': > Duration: N/A, bitrate: N/A > Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 > DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc > Stream mapping: > Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) > Press [q] to stop, [?] for help > Output #0, mpegts, to 'test8_1080i_mp2_2.ts': > Metadata: > encoder : Lavf57.72.100 > Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first > (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k > tbn, 29.97 tbc > Metadata: > encoder : Lavc57.92.100 mpeg2video > Side data: > cpb: bitrate max/min/avg: 0/0/20 buffer size: 0 vbv_delay: -1 > > -- > > As can be seen, before the patch, after decoding the .h264 file and then > re-encoding it as mpeg2video in an mpegts container, the interlaced aspect of > the video has been lost in the output, and it is now effectively 1080p29.97, > although the video hasn't actually been converted to progressive. ffmpeg > simply thinks that the video is progressive when it is not. With the patch, > the interlaced aspect is not lost and propagates to the output. So, this > conclusively demonstrates that the issue has nothing to do with Blackmagic > and is a more general issue with interlaced video and decoding. > > I can make the input file available if that would be helpful. > > Anyway, it would be great if this bug fix could make it into ffmpeg. > > Thanks, > Aaron Levinson I've provided a new version of the patch. When I created the first version of the patch on March 26th, this was the first patch that I submitted to ffmpeg, and some aspects were rough. I had indicat
Re: [FFmpeg-devel] [PATCH] This fixes ISO date formatissue when manifest created by this muxer is not playable in most players. This ensures compatibility with dash standard. Tested on many players (d
On 5/2/2017 2:29 PM, wm4 wrote: On Tue, 2 May 2017 14:17:33 -0700 Aaron Levinson <alevi...@aracnet.com> wrote: On 5/1/2017 11:06 PM, MFojtak wrote: Currently this muxer does not work at all. I don't know if 000Z would make it compatible with more player as I don't know any. However, adding Z makes it compatible with most popular ones like dash.js and shaka. Having this muxer working properly would bring more attention to it and maybe in the future somebody tests it with more players. But for now I suggest to roll out the change and "unblock" this muxer for some real wold use. Also, it would be great to make this muxer codec and container agnostic as it works with h264 and mp4 only. But again, nobody would bother if the muxer doesn't work at all with browsers. I think your original patch is fine, and I only offered the possibility that adding ".000Z" might be even better than just "Z". I don't have push privileges, so I can't commit your patch, but hopefully someone else will do so. Before someone pushes it, please fix the commit message. MFojtak: you can make it more likely that this patch will be pushed sooner by altering the commit message (shortening it--plenty of examples on the e-mail list) and then putting any additional information later. Something like: " avformat/dashenc: Adjusts ISO date formatting for improved compatibility with most players Adjusts ISO date formatting " Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: hold old key info when append list
On 5/4/2017 9:15 PM, Steven Liu wrote: 2017-05-03 9:49 GMT+08:00 Aaron Levinson <alevi...@aracnet.com>: On 4/27/2017 7:21 PM, Steven Liu wrote: 2017-04-26 7:30 GMT+08:00 Steven Liu <l...@chinaffmpeg.org>: fix ticket id: #6353 Signed-off-by: Steven Liu <l...@chinaffmpeg.org> --- libavformat/hlsenc.c | 24 1 file changed, 24 insertions(+) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 27c8e3355d..3ec0f330fb 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const char *url) int64_t new_start_pos; char line[1024]; const char *ptr; +const char *end; if ((ret = ffio_open_whitelist(, url, AVIO_FLAG_READ, >interrupt_callback, NULL, @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const char *url) } else if (av_strstart(line, "#EXTINF:", )) { is_segment = 1; hls->duration = atof(ptr); +} else if (av_stristart(line, "#EXT-X-KEY:", )) { +ptr = av_stristr(line, "URI=\""); +if (ptr) { +ptr += strlen("URI=\""); +end = av_stristr(ptr, ","); +if (end) { +av_strlcpy(hls->key_uri, ptr, end - ptr); +} else { +av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); I know that this patch has already been applied (although it didn't get any reviews on the ML), but I think that there are some issues with it. First, it seems that both av_strlcpy() cases above will copy the terminating '\"' character into hls->key_uri. Perhaps that won't cause an issue in practice, but it is likely not the intended result. Second, with both av_strlcpy() calls that use a length of end - ptr, this could in theory result in a buffer overrun depending on how long the URI is, since the destination buffers have a fixed size. This issue is prevented in the second call to av_strlcpy(), since it uses sizeof(hls->key_uri), but it is a potential issue with the first calls (note that this comment applies to the IV=0x code below). If you want to use av_strlcpy(), either make sure that end - ptr is less than or equal to sizeof(hls->key_uri) or do something like *end = 0 first and then use av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)). In addition, based on the EXT-X-KEY example at https://developer.apple.com/library/content/technotes/tn2288/_index.html , it would appear that an EXT-X-KEY declaration may span multiple lines. Your solution will not work properly in this case. Hi Aaron, I think the libavformat/hls.c maybe have the same problem, because both of hlsenc and hls use read_chomp_line to read one line, that need check the '\' tail a line, and read the next line into a buffer, that maybe better, is that right? Yes, I was thinking the same thing, that read_chomp_line() needs to be enhanced to deal with declarations that span multiple lines. That probably belongs in a separate patch though, even if it is only relevant for EXT-X-KEY. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC]lavu/timecode: Increase AV_TIMECODE_STR_SIZE.
On 5/4/2017 4:27 PM, Carl Eugen Hoyos wrote: Hi! Attached patch is one possibility to fix the following warning with gcc 7: libavutil/timecode.c: In function ‘av_timecode_make_string’: libavutil/timecode.c:103:60: warning: ‘%02d’ directive output may be truncated writing between 2 and 10 bytes into a region of size between 0 and 7 [-Wformat-truncation=] snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^~~~ libavutil/timecode.c:103:41: note: directive argument in the range [0, 2147483647] snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^~~~ libavutil/timecode.c:103:5: note: ‘snprintf’ output between 12 and 32 bytes into a destination of size 16 snprintf(buf, AV_TIMECODE_STR_SIZE, "%s%02d:%02d:%02d%c%02d", ^ neg ? "-" : "", ~~~ hh, mm, ss, drop ? ';' : ':', ff); ~ Several similar warnings are printed, an alternative is to disable the warning. The warning seemed wrong on first sight but may actually be correct, we accept invalid fps for timecode. Regarding the change in your patch: -#define AV_TIMECODE_STR_SIZE 16 +#define AV_TIMECODE_STR_SIZE 23 It seems like 23 characters wouldn't be sufficient based on the warning: "output between 12 and 32 bytes into a destination of size 16". I would guess that you would need at least 32 characters and perhaps one more (?) for the terminating null character to avoid that warning. Even if in practice, it will never use 32 characters, its not a big deal to have a little waste here. The buffer size should match the format specification. An alternative would be to use "%02hd" or "%02hhd", but doing so would require casts or using different types for hh, mm, etc. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC]lavu/opt: Use && instead of * in boolean expression
On 5/4/2017 4:32 PM, Carl Eugen Hoyos wrote: > Hi! > > It may be better to disable the warning. > > Carl Eugen > > -num = den ? num * intnum / den : (num * intnum ? INFINITY : NAN); > +num = den ? num * intnum / den : (num && intnum ? INFINITY : NAN); In order to preserve the original logic, why not do the following: +num = den ? num * intnum / den : (((num * intnum) != 0) ? INFINITY : NAN); Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 0/9] Removing HEVCContext depencencies
James, Can you document which patches depend on other patches in this patch set to make it easier to review this patch set in chunks? Thanks, Aaron Levinson On 5/2/2017 2:55 PM, James Almer wrote: Some changes and fixes suggested on IRC. Some patches are v3 as they have been sent before outside the previous set. James Almer (9): hevcdec: remove HEVCContext usage from hevc_sei hevcdec: move SEI message parsing into a separate header hevcdec: remove HEVCContext usage from ff_hevc_compute_poc() hevcdec: move SliceHeader struct definition to hevc_ps hevc_parser: use ff_h2645_packet_split() to parse NAL units hevc_parser: remove HEVCContext usage hevc_parser: move slice header parsing to its own function hevc_parse: decode SEI message NALUs in extradata doc/libav_merge: remove line about ADVANCED_PARSER doc/libav-merge.txt| 1 - libavcodec/Makefile| 4 +- libavcodec/hevc_parse.c| 21 ++- libavcodec/hevc_parse.h| 7 +- libavcodec/hevc_parser.c | 415 - libavcodec/hevc_ps.c | 23 +++ libavcodec/hevc_ps.h | 88 ++ libavcodec/hevc_refs.c | 27 +-- libavcodec/hevc_sei.c | 193 + libavcodec/hevc_sei.h | 125 ++ libavcodec/hevcdec.c | 94 +- libavcodec/hevcdec.h | 137 +-- libavcodec/mediacodecdec.c | 4 +- 13 files changed, 532 insertions(+), 607 deletions(-) create mode 100644 libavcodec/hevc_sei.h ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined
On 4/21/2017 2:03 PM, James Almer wrote: On 4/21/2017 12:09 PM, Michael Niedermayer wrote: On Thu, Apr 20, 2017 at 11:30:13PM -0700, Aaron Levinson wrote: From 4f27e910aca6dae6642b4d73cf07fa0d6c4b1618 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Thu, 20 Apr 2017 23:20:20 -0700 Subject: [PATCH] Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined Purpose: Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined. If FF_API_LAVF_AVCTX is defined, AVStream::codec is set to an AVCodecContext object, and this wasn't being deallocated properly when the AVStream object was freed. While FF_API_LAVF_AVCTX has to do with deprecated functionality, in practice, it will always be defined for typical builds currently, since it is defined in libavformat\version.h if LIBAVFORMAT_VERSION_MAJOR is less than 58, and LIBAVFORMAT_VERSION_MAJOR is currently set to 57. Comments: -- libavformat/utils.c: Now using avcodec_free_context() to properly deallocate st->codec in free_stream() if FF_API_LAVF_AVCTX is defined. --- libavformat/utils.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) please use "make fate" to test your changes This one causes many all? tests to segfault The issue is coded_side_data in AVStream->codec. ffmpeg.c calls avcodec_copy_context() to copy the encoder AVCodecContext to the output's AVStream->codec because the latter is still needed by some internal code in lavf/utils.c and such. avcodec_copy_context() copies the coded_side_data pointer as part of its memcpy call but not the buffers, and by the time avcodec_free_context() is called for AVStream->codec the buffers said pointer points to was already freed by the avcodec_free_context() call for the encoder AVCodecContext. The proper solution: Remove the avcodec_copy_context() call in ffmpeg.c and make libavformat stop needing AVStream->codec internally. It should use AVStream->internal->avctx instead. Even if this is not done now, it will anyway when the deprecation period ends and it's time to remove AVStream->codec. The easy but ugly solution until the above is done: Copy the coded_side_data buffers in avcodec_copy_context(). One could argue that by definition avcodec_copy_context() should copy said side data, but the function is clearly marked as "do not use, its behavior is ill defined" and the user is asked instead to copy using an intermediary AVCodecParameters context instead. Now that James Almer's patches pertaining to avcodec_copy_context() have been applied, perhaps the patch I submitted a couple of weeks ago can be considered anew. I've confirmed that make fate SAMPLES=fate-suite/ (64-bit, Linux) completes successfully with the patch that I submitted. Thanks, Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] doc: Enhanced fate.texi to reference Windows line endings section from git-howto.texi
From 345a98d31c6c758f0f82edab1b502cd8aeb3354b Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Wed, 3 May 2017 12:38:39 -0700 Subject: [PATCH] doc: Enhanced fate.texi to reference Windows line endings section from git-howto.texi Purpose: Enhanced fate.texi to indicate that FATE will fail if files are checked out with Windows line endings (CRLF) and added reference to Windows line endings anchor in git-howto.texi for how to address. Also added Windows line endings anchor in git-howto.texi. The exact problem caused by CRLF endings occurs with check-source.sh, which uses git grep. git grep doesn't appear to be able to handle CRLF line endings. Note: thanks to Hendrik Leppkes for suggestion that the issue with FATE failing was caused by CRLF line endings. Signed-off-by: Aaron Levinson <alevi...@aracnet.com> --- doc/fate.texi | 5 + doc/git-howto.texi | 1 + 2 files changed, 6 insertions(+) diff --git a/doc/fate.texi b/doc/fate.texi index 7a96c25..c498336 100644 --- a/doc/fate.texi +++ b/doc/fate.texi @@ -77,6 +77,11 @@ FATE_SAMPLES=fate-suite/ make fate @float NOTE Do not put a '~' character in the samples path to indicate a home directory. Because of shell nuances, this will cause FATE to fail. + +In addition, FATE will fail if files are checked out from git such +that @kbd{@key{CR}@key{LF}} is used for line endings. Please refer to +@ref{Windows line endings,,Windows line endings,git-howto} in the +FFmpeg documentation for more information. @end float To use a custom wrapper to run the test, pass @option{--target-exec} to diff --git a/doc/git-howto.texi b/doc/git-howto.texi index 2b4fb80..f2b6182 100644 --- a/doc/git-howto.texi +++ b/doc/git-howto.texi @@ -80,6 +80,7 @@ create patches after making a read-only ffmpeg-web clone: git clone git://ffmpeg.org/ffmpeg-web @end example +@anchor{Windows line endings} Make sure that you do not have Windows line endings in your checkouts, otherwise you may experience spurious compilation failures. One way to achieve this is to run -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true
On 4/22/2017 12:22 PM, Aaron Levinson wrote: On 4/22/2017 12:09 PM, Hendrik Leppkes wrote: On Sat, Apr 22, 2017 at 9:07 PM, Aaron Levinson <alevi...@aracnet.com> wrote: On 4/22/2017 2:16 AM, Clément Bœsch wrote: On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote: [...] This is documented in doc/git-howto.texi, isn't it? Yes it is, but just because it can be found elsewhere in the documentation doesn't mean there isn't benefit to having a fallback elsewhere in the documentation. Can't we just make it point there, instead of explaining it again? OK, I'll do that. I'm abandoning this patch and will submit a new one shortly with a new title. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/concatdec: port to the new bitstream filter API
I don't seem to have the original e-mail in my inbox anymore, so I'll respond to wm4's response. Overall, the new code is a lot cleaner and easier to understand than the existing code. See below for more. Aaron Levinson On 5/2/2017 5:04 PM, wm4 wrote: On Fri, 28 Apr 2017 21:27:56 -0300 James Almer <jamr...@gmail.com> wrote: Signed-off-by: James Almer <jamr...@gmail.com> --- libavformat/concatdec.c | 94 - 1 file changed, 30 insertions(+), 64 deletions(-) diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c index dd52e4d366..fa9443ff78 100644 --- a/libavformat/concatdec.c +++ b/libavformat/concatdec.c @@ -34,8 +34,7 @@ typedef enum ConcatMatchMode { } ConcatMatchMode; typedef struct ConcatStream { -AVBitStreamFilterContext *bsf; -AVCodecContext *avctx; +AVBSFContext *bsf; int out_stream_index; } ConcatStream; @@ -196,7 +195,8 @@ static int detect_stream_specific(AVFormatContext *avf, int idx) ConcatContext *cat = avf->priv_data; AVStream *st = cat->avf->streams[idx]; ConcatStream *cs = >cur_file->streams[idx]; -AVBitStreamFilterContext *bsf; +const AVBitStreamFilter *filter; +AVBSFContext *bsf; int ret; if (cat->auto_convert && st->codecpar->codec_id == AV_CODEC_ID_H264) { @@ -206,29 +206,28 @@ static int detect_stream_specific(AVFormatContext *avf, int idx) return 0; av_log(cat->avf, AV_LOG_INFO, "Auto-inserting h264_mp4toannexb bitstream filter\n"); -if (!(bsf = av_bitstream_filter_init("h264_mp4toannexb"))) { +filter = av_bsf_get_by_name("h264_mp4toannexb"); +if (!filter) { av_log(avf, AV_LOG_ERROR, "h264_mp4toannexb bitstream filter " "required for H.264 streams\n"); return AVERROR_BSF_NOT_FOUND; } +ret = av_bsf_alloc(filter, ); +if (ret < 0) +return ret; cs->bsf = bsf; -cs->avctx = avcodec_alloc_context3(NULL); -if (!cs->avctx) -return AVERROR(ENOMEM); - -/* This really should be part of the bsf work. - Note: input bitstream filtering will not work with bsf that - create extradata from the first packet. */ -av_freep(>codecpar->extradata); -st->codecpar->extradata_size = 0; +ret = avcodec_parameters_copy(bsf->par_in, st->codecpar); +if (ret < 0) + return ret; -ret = avcodec_parameters_to_context(cs->avctx, st->codecpar); -if (ret < 0) { -avcodec_free_context(>avctx); +ret = av_bsf_init(bsf); +if (ret < 0) return ret; -} +ret = avcodec_parameters_copy(st->codecpar, bsf->par_out); +if (ret < 0) +return ret; } return 0; } @@ -291,8 +290,11 @@ static int match_streams(AVFormatContext *avf) memset(map + cat->cur_file->nb_streams, 0, (cat->avf->nb_streams - cat->cur_file->nb_streams) * sizeof(*map)); -for (i = cat->cur_file->nb_streams; i < cat->avf->nb_streams; i++) +for (i = cat->cur_file->nb_streams; i < cat->avf->nb_streams; i++) { map[i].out_stream_index = -1; +if ((ret = detect_stream_specific(avf, i)) < 0) +return ret; +} switch (cat->stream_match_mode) { case MATCH_ONE_TO_ONE: ret = match_streams_one_to_one(avf); @@ -305,9 +307,6 @@ static int match_streams(AVFormatContext *avf) } if (ret < 0) return ret; -for (i = cat->cur_file->nb_streams; i < cat->avf->nb_streams; i++) -if ((ret = detect_stream_specific(avf, i)) < 0) -return ret; cat->cur_file->nb_streams = cat->avf->nb_streams; return 0; } This just moves already existing code around. While it is unclear to me why this is being done (a comment and/or log message would help), I would suspect that this particular change is unrelated to the purpose of this patch: "port to the new bitstream filter API". @@ -370,10 +369,8 @@ static int concat_read_close(AVFormatContext *avf) for (i = 0; i < cat->nb_files; i++) { av_freep(>files[i].url); for (j = 0; j < cat->files[i].nb_streams; j++) { -if (cat->files[i].streams[j].avctx) -avcodec_free_context(>files[i].streams[j].avctx); if (cat->files[i].streams[j].bsf) -av_bitstream_filter_close(cat->files[i].streams[j].bsf); +av_bsf_free(>files[i].streams[j].bsf); } av_freep(>files[i].streams); av_dict_free(>files[i].metadata); @@ -524,56 +521,25 @@ static int open_next_file(A
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: hold old key info when append list
On 4/27/2017 7:21 PM, Steven Liu wrote: 2017-04-26 7:30 GMT+08:00 Steven Liu <l...@chinaffmpeg.org>: fix ticket id: #6353 Signed-off-by: Steven Liu <l...@chinaffmpeg.org> --- libavformat/hlsenc.c | 24 1 file changed, 24 insertions(+) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 27c8e3355d..3ec0f330fb 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const char *url) int64_t new_start_pos; char line[1024]; const char *ptr; +const char *end; if ((ret = ffio_open_whitelist(, url, AVIO_FLAG_READ, >interrupt_callback, NULL, @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const char *url) } else if (av_strstart(line, "#EXTINF:", )) { is_segment = 1; hls->duration = atof(ptr); +} else if (av_stristart(line, "#EXT-X-KEY:", )) { +ptr = av_stristr(line, "URI=\""); +if (ptr) { +ptr += strlen("URI=\""); +end = av_stristr(ptr, ","); +if (end) { +av_strlcpy(hls->key_uri, ptr, end - ptr); +} else { +av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); I know that this patch has already been applied (although it didn't get any reviews on the ML), but I think that there are some issues with it. First, it seems that both av_strlcpy() cases above will copy the terminating '\"' character into hls->key_uri. Perhaps that won't cause an issue in practice, but it is likely not the intended result. Second, with both av_strlcpy() calls that use a length of end - ptr, this could in theory result in a buffer overrun depending on how long the URI is, since the destination buffers have a fixed size. This issue is prevented in the second call to av_strlcpy(), since it uses sizeof(hls->key_uri), but it is a potential issue with the first calls (note that this comment applies to the IV=0x code below). If you want to use av_strlcpy(), either make sure that end - ptr is less than or equal to sizeof(hls->key_uri) or do something like *end = 0 first and then use av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)). In addition, based on the EXT-X-KEY example at https://developer.apple.com/library/content/technotes/tn2288/_index.html , it would appear that an EXT-X-KEY declaration may span multiple lines. Your solution will not work properly in this case. Aaron Levinson +} +} + +ptr = av_stristr(line, "IV=0x"); +if (ptr) { +ptr += strlen("IV=0x"); +end = av_stristr(ptr, ","); +if (end) { +av_strlcpy(hls->iv_string, ptr, end - ptr); +} else { +av_strlcpy(hls->iv_string, ptr, sizeof(hls->iv_string)); +} +} + } else if (av_strstart(line, "#", NULL)) { continue; } else if (line[0]) { -- 2.11.0 (Apple Git-81) Applied! Thanks ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]configure: Fix libopus detection
On 4/25/2017 1:19 AM, Carl Eugen Hoyos wrote: 2017-04-13 1:08 GMT+02:00 Carl Eugen Hoyos <ceffm...@gmail.com>: 2017-03-30 1:52 GMT+02:00 James Almer <jamr...@gmail.com>: On 3/29/2017 7:47 PM, Carl Eugen Hoyos wrote: Hi! Attached patch fixes a compilation error here. Please test for success, Carl Eugen 0001-configure-Fix-libopus-detection.patch From 600b568651c60f8de609f211c814b5cd0640e584 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceho...@ag.or.at> Date: Thu, 30 Mar 2017 00:45:06 +0200 Subject: [PATCH] configure: Fix libopus detection. Avoids a compilation error for old libopus. Regression since 37941878 --- configure |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index a84b126..76a287e 100755 --- a/configure +++ b/configure @@ -5797,7 +5797,7 @@ enabled libopenjpeg && { { check_lib openjpeg-2.1/openjpeg.h opj_version - { check_lib openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC && add_cppflags -DOPJ_STATIC; } || die "ERROR: libopenjpeg not found"; } enabled libopenmpt&& require_pkg_config "libopenmpt >= 0.2.6557" libopenmpt/libopenmpt.h openmpt_module_create -enabled libopus && require_pkg_config opus opus_multistream.h opus_multistream_decoder_create +enabled libopus && require_pkg_config opus opus_multistream.h opus_multistream_surround_encoder_create Should be ok, but strictly speaking, this function is needed by the encoder and not the decoder. Something like enabled libopus && { enabled libopus_decoder && { require_pkg_config opus opus_multistream.h opus_multistream_decoder_create } enabled libopus_encoder && { use_pkg_config "opus >= 1.1" opus_multistream.h opus_multistream_surround_encoder_create || disable libopus_encoder; } } Please commit this if you prefer it. Ping. Perhaps you could submit a new patch that modifies configure in the fashion suggested by James Almer. Technically, it is possible to enable the libopus decoder independently of the libopus encoder, and if that is done, it won't build libopusenc.c. But, with your original patch, it won't even permit configure to succeed even though such a build would be possible. It is true that the approach currently taken in configure doesn't distinguish between encoding and decoding, but the patch as originally proposed just trades decoding API requirements for encoding API requirements. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] This fixes ISO date formatissue when manifest created by this muxer is not playable in most players. This ensures compatibility with dash standard. Tested on many players (d
On 5/1/2017 11:06 PM, MFojtak wrote: Currently this muxer does not work at all. I don't know if 000Z would make it compatible with more player as I don't know any. However, adding Z makes it compatible with most popular ones like dash.js and shaka. Having this muxer working properly would bring more attention to it and maybe in the future somebody tests it with more players. But for now I suggest to roll out the change and "unblock" this muxer for some real wold use. Also, it would be great to make this muxer codec and container agnostic as it works with h264 and mp4 only. But again, nobody would bother if the muxer doesn't work at all with browsers. I think your original patch is fine, and I only offered the possibility that adding ".000Z" might be even better than just "Z". I don't have push privileges, so I can't commit your patch, but hopefully someone else will do so. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]compat/strtod: Add missing const qualifiers
On 5/1/2017 1:51 AM, Carl Eugen Hoyos wrote: Hi! Even without the casts, the patch reduces the number of warnings shown when compiling compat/strtod from seven to three. Please comment, Carl Eugen LGTM Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] This fixes ISO date formatissue when manifest created by this muxer is not playable in most players. This ensures compatibility with dash standard. Tested on many players (d
On 4/26/2017 4:27 AM, mfojtak wrote: --- libavformat/dashenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c index 6232c70..fe1d6c2 100644 --- a/libavformat/dashenc.c +++ b/libavformat/dashenc.c @@ -433,7 +433,7 @@ static void format_date_now(char *buf, int size) struct tm *ptm, tmbuf; ptm = gmtime_r(, ); if (ptm) { -if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S", ptm)) +if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%SZ", ptm)) buf[0] = '\0'; } } This change appears to be correct. I wasn't previously knowledgeable about the 'Z' suffix, but I looked into it and it is documented in ISO 8601 (https://en.wikipedia.org/wiki/ISO_8601 and also http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.15 ). On a separate note, the actual format is: -MM-DDTHH:mm:ss.sssZ . The ".sss" part is missing from this implementation, which represents milliseconds. According to the specification, ".sss" may be absent, but maybe it would work with even more players if it were included. Technically, the specification states that an absent time-zone offset should be treated as 'Z', which indicates that the code was already compliant without the 'Z', even if it didn't work with most players. strftime() doesn't handle milliseconds, but perhaps it ought to populate milliseconds anyway as follows: if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S.000Z", ptm)) Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/options: do a more thorough clean up in avcodec_copy_context()
On 4/26/2017 12:47 PM, James Almer wrote: On 4/26/2017 2:46 AM, Aaron Levinson wrote: On 4/24/2017 3:47 PM, James Almer wrote: Free coded_frame, coded_side_data and unref hw_device_ctx to prevent potential leaks. Signed-off-by: James Almer <jamr...@gmail.com> --- libavcodec/options.c | 15 +++ 1 file changed, 15 insertions(+) diff --git a/libavcodec/options.c b/libavcodec/options.c index b98da9378a..82e12179a6 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) #if FF_API_COPY_CONTEXT static void copy_context_reset(AVCodecContext *avctx) { +int i; + av_opt_free(avctx); +#if FF_API_CODED_FRAME +FF_DISABLE_DEPRECATION_WARNINGS +av_frame_free(>coded_frame); +FF_ENABLE_DEPRECATION_WARNINGS +#endif av_freep(>rc_override); av_freep(>intra_matrix); av_freep(>inter_matrix); av_freep(>extradata); av_freep(>subtitle_header); av_buffer_unref(>hw_frames_ctx); +av_buffer_unref(>hw_device_ctx); +for (i = 0; i < avctx->nb_coded_side_data; i++) +av_freep(>coded_side_data[i].data); +av_freep(>coded_side_data); avctx->subtitle_header_size = 0; +avctx->nb_coded_side_data = 0; avctx->extradata_size = 0; } @@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS /* reallocate values that should be allocated separately */ dest->extradata = NULL; +dest->coded_side_data = NULL; dest->intra_matrix= NULL; dest->inter_matrix= NULL; dest->rc_override = NULL; dest->subtitle_header = NULL; dest->hw_frames_ctx = NULL; +dest->hw_device_ctx = NULL; +dest->nb_coded_side_data = 0; #define alloc_and_copy_or_fail(obj, size, pad) \ if (src->obj && size > 0) { \ I'm not sure if this patch is intended to be a replacement for your last "2/2" patch, but this version is missing the the coded_side_data population code that was in the first version of your "2/2" patch. Aaron Levinson It is. I'm keeping the functionality of the function as is, and only making sure cleanup is complete to prevent leaks. This function is deprecated and shouldn't be used, so i'm not going to make it copy even more stuff. I've reviewed both patches 1 and 2, and both are good to commit. Technically, the behavior of the code is changed by zeroing out coded_side_data and nb_coded_side_data after doing the memcpy() call, even though, by not doing a copy of coded_side_data() previously, this resulted in the same coded_side_data pointer being owned by two AVCodecContext objects (assuming that there is any coded side data in the source AVCodecContext object). This was a bug, but if there is any code that depends on there being valid coded side data in the copied context, then this could result in problems. However, I've surveyed the code in ffmpeg proper, and I actually can find no uses of coded_side_data in AVCodecContext objects other than the code in ffmpeg.c to copy coded side data into the stream, and it is the copy of the coded side data in the stream that is actually used. This entire procedure of populating coded_side_data in AVCodecContext objects for it to be later copied into an AVStream object where it is actually used is a bit kludgy and could use some work. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: fix CID 1405135
On 4/25/2017 4:47 PM, Steven Liu wrote: CID: 1405135 Signed-off-by: Steven Liu <l...@chinaffmpeg.org> --- libavformat/hlsenc.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 3ec0f330fb..b7aafb73da 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -394,11 +394,11 @@ static int do_encrypt(AVFormatContext *s) av_strlcat(hls->key_basename, ".key", len); if (hls->key_url) { -strncpy(hls->key_file, hls->key_url, sizeof(hls->key_file)); -strncpy(hls->key_uri, hls->key_url, sizeof(hls->key_uri)); +av_strlcpy(hls->key_file, hls->key_url, strlen(hls->key_url)); +av_strlcpy(hls->key_uri, hls->key_url, strlen(hls->key_url)); } else { -strncpy(hls->key_file, hls->key_basename, sizeof(hls->key_file)); -strncpy(hls->key_uri, hls->key_basename, sizeof(hls->key_uri)); +av_strlcpy(hls->key_file, hls->key_basename, strlen(hls->key_basename)); +av_strlcpy(hls->key_uri, hls->key_basename, strlen(hls->key_basename)); If either hls->key_url or hls->key_basename, depending on which path the code takes, is longer in length than key_file or key_uri (both of size LINE_BUFFER_SIZE + 1), then the calls to av_strlcpy() will write past the end of the buffers. As key_url corresponds to a user-passed option, and I think the same may be true for key_basename based on the way it is formed, this in theory could be a problem, although it is unlikely in practice since LINE_BUFFER_SIZE is 1024. While ideally there would be a sanity check somewhere in the code to make sure those URLs aren't too long, an alternative is to copy up to max LINE_BUFFER_SIZE characters and make sure the string is null-terminated in that case. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/options: do a more thorough clean up in avcodec_copy_context()
On 4/24/2017 3:47 PM, James Almer wrote: Free coded_frame, coded_side_data and unref hw_device_ctx to prevent potential leaks. Signed-off-by: James Almer <jamr...@gmail.com> --- libavcodec/options.c | 15 +++ 1 file changed, 15 insertions(+) diff --git a/libavcodec/options.c b/libavcodec/options.c index b98da9378a..82e12179a6 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) #if FF_API_COPY_CONTEXT static void copy_context_reset(AVCodecContext *avctx) { +int i; + av_opt_free(avctx); +#if FF_API_CODED_FRAME +FF_DISABLE_DEPRECATION_WARNINGS +av_frame_free(>coded_frame); +FF_ENABLE_DEPRECATION_WARNINGS +#endif av_freep(>rc_override); av_freep(>intra_matrix); av_freep(>inter_matrix); av_freep(>extradata); av_freep(>subtitle_header); av_buffer_unref(>hw_frames_ctx); +av_buffer_unref(>hw_device_ctx); +for (i = 0; i < avctx->nb_coded_side_data; i++) +av_freep(>coded_side_data[i].data); +av_freep(>coded_side_data); avctx->subtitle_header_size = 0; +avctx->nb_coded_side_data = 0; avctx->extradata_size = 0; } @@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS /* reallocate values that should be allocated separately */ dest->extradata = NULL; +dest->coded_side_data = NULL; dest->intra_matrix= NULL; dest->inter_matrix= NULL; dest->rc_override = NULL; dest->subtitle_header = NULL; dest->hw_frames_ctx = NULL; +dest->hw_device_ctx = NULL; +dest->nb_coded_side_data = 0; #define alloc_and_copy_or_fail(obj, size, pad) \ if (src->obj && size > 0) { \ I'm not sure if this patch is intended to be a replacement for your last "2/2" patch, but this version is missing the the coded_side_data population code that was in the first version of your "2/2" patch. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [rfc] coc violation tribunal
On 4/24/2017 5:05 PM, Compn wrote: as a few developers have wondered... how is our project to judge, report and punish coc violations? since we had a vote to approve of the COC, we will probably need another vote to approve of the COC rules. do you want group consensus? how big of a group? whos in the group? who wants to do a bunch of crapwork telling devs to be nice in the sandbox and stop throwing sand at each other or they'll get a timeout? do you want irc/ml admins to handle it? e.g. lou and me set moderation flags on developers and delete mails that are not development related? who judges the judges? what if we scare off devs who get frustrated that the COC was not fully fairly applied to everyone? e.g. one dev gets moderated while another skates ? thats where the strife gains momentum. thoughts? I think it would be best to set this aside for now. I think the reason things flared up again is nothing was ever resolved the last time around ("Re: [FFmpeg-devel] [PATCH 1/2] opus_pvq: add resynth support and band encoding cost function"), and instead, it just sort of died down. There was such animosity displayed on that thread from specific individuals towards other specific individuals. No one benefits from that, least of all the people writing it or the people being targeted. I know what it is like to be in an environment in which co-workers basically hate each other, and it isn't pleasant, and I think it is reasonable to consider other people on this e-mail list as sort of co-workers. Perhaps what would be helpful would be to get these individuals together on a private IRC channel with someone who perhaps knows both of them to neutrally moderate and have them air their differences and ideally come to some sort of resolution and move forward. Its one thing to fire off an e-mail--its another to say things to someone's "face", so to speak. I'd also suggest, when passions are up, to step aside and then maybe come back to it later. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] Reviewing merges
On 4/23/2017 7:07 PM, Michael Niedermayer wrote: Hi all Should changes ported from libav (what we call merges) be reviewed before being pushed? I've asked about this on IRC (#ffmpeg-devel). The overall consensus there, at least at the time I asked it, is there is an expectation that such changes have already been properly reviewed for libav, so they are likely to be okay for ffmpeg, and while some issues have crept into ffmpeg as a result of these merges, the merge process has gone smoothly overall. As the code bases diverge more and more, reviewing ports from libav would be nice, but based on the rate at which patches posted to ffmpeg-devel are reviewed, I think that could overload the code review resources on ffmpeg-devel. Plus, the expertise may not exist to properly review some of the ported changes on ffmpeg-devel. For example, I submitted a patch to fix a bug with QuickSync interlaced video to ffmpeg-devel on 4/13/2017. The change mostly reverted some of the QSV code in ffmpeg back to an earlier state. Interlaced video QSV encoding used to work in ffmpeg, but this was broken in October 2016 as a result of a merge with libav. However, I was asked to submit the patch to libav instead because QSV development is more active on libav-devel. I did that, and the patch finally made it into libav today. And, my hope is that it will be merged into ffmpeg--that is, it seemed like the most readily available path to getting this code into ffmpeg was to contribute it to libav first. But, if this patch has to pass yet another round of code reviews, code reviews that weren't forthcoming in the first place on ffmpeg-devel when I originally submitted the patch, then that could be problematic. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/options: copy the coded_side_data in avcodec_copy_context()
On 4/22/2017 10:26 AM, James Almer wrote: Signed-off-by: James Almer <jamr...@gmail.com> --- libavcodec/options.c | 28 1 file changed, 28 insertions(+) diff --git a/libavcodec/options.c b/libavcodec/options.c index b98da9378a..c721aa8d43 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -190,15 +190,21 @@ void avcodec_free_context(AVCodecContext **pavctx) #if FF_API_COPY_CONTEXT static void copy_context_reset(AVCodecContext *avctx) { +int i; + av_opt_free(avctx); av_freep(>rc_override); av_freep(>intra_matrix); av_freep(>inter_matrix); av_freep(>extradata); av_freep(>subtitle_header); +for (i = 0; i < avctx->nb_coded_side_data; i++) +av_freep(>coded_side_data[i].data); +av_freep(>coded_side_data); av_buffer_unref(>hw_frames_ctx); avctx->subtitle_header_size = 0; avctx->extradata_size = 0; +avctx->nb_coded_side_data = 0; } int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src) @@ -262,6 +268,28 @@ FF_ENABLE_DEPRECATION_WARNINGS alloc_and_copy_or_fail(subtitle_header, src->subtitle_header_size, 1); av_assert0(dest->subtitle_header_size == src->subtitle_header_size); #undef alloc_and_copy_or_fail +if (src->nb_coded_side_data) { +int i; + +dest->nb_coded_side_data = 0; +dest->coded_side_data = av_realloc_array(NULL, src->nb_coded_side_data, + sizeof(*dest->coded_side_data)); +if (!dest->coded_side_data) +goto fail; + +for (i = 0; i < src->nb_coded_side_data; i++) { +const AVPacketSideData *sd_src = >coded_side_data[i]; +AVPacketSideData *sd_dst = >coded_side_data[i]; + +sd_dst->data = av_malloc(sd_src->size); +if (!sd_dst->data) +goto fail; +memcpy(sd_dst->data, sd_src->data, sd_src->size); +sd_dst->size = sd_src->size; +sd_dst->type = sd_src->type; +dest->nb_coded_side_data++; +} +} if (src->hw_frames_ctx) { dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx); If src has coded_side_data and nb_coded_side_data set to non-zero values, as a result of the earlier call to memcpy(), it will copy those values into dest. Then, if it does a goto to fail early, as a result of your changes to copy_context_reset(), it will call av_freep() on those elements in dest, even though they are owned by src. After doing the memcpy() call, I think you can fix this by setting dest->coded_side_data to NULL and dest->nb_coded_side_data to 0. Everything else looks good. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/options: factorize avcodec_copy_context cleanup code
On 4/22/2017 10:26 AM, James Almer wrote: Signed-off-by: James Almer <jamr...@gmail.com> --- libavcodec/options.c | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/libavcodec/options.c b/libavcodec/options.c index 7bdb0be5af..b98da9378a 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -188,6 +188,19 @@ void avcodec_free_context(AVCodecContext **pavctx) } #if FF_API_COPY_CONTEXT +static void copy_context_reset(AVCodecContext *avctx) +{ +av_opt_free(avctx); +av_freep(>rc_override); +av_freep(>intra_matrix); +av_freep(>inter_matrix); +av_freep(>extradata); +av_freep(>subtitle_header); +av_buffer_unref(>hw_frames_ctx); +avctx->subtitle_header_size = 0; +avctx->extradata_size = 0; +} + int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src) { const AVCodec *orig_codec = dest->codec; @@ -200,12 +213,7 @@ int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src) return AVERROR(EINVAL); } -av_opt_free(dest); -av_freep(>rc_override); -av_freep(>intra_matrix); -av_freep(>inter_matrix); -av_freep(>extradata); -av_freep(>subtitle_header); +copy_context_reset(dest); memcpy(dest, src, sizeof(*dest)); av_opt_copy(dest, src); @@ -264,15 +272,7 @@ FF_ENABLE_DEPRECATION_WARNINGS return 0; fail: -av_freep(>subtitle_header); -av_freep(>rc_override); -av_freep(>intra_matrix); -av_freep(>inter_matrix); -av_freep(>extradata); -av_buffer_unref(>hw_frames_ctx); -dest->subtitle_header_size = 0; -dest->extradata_size = 0; -av_opt_free(dest); +copy_context_reset(dest); return AVERROR(ENOMEM); } #endif There is one real difference in the behavior--it will call av_buffer_unref(>hw_frames_ctx) the first time around, and this wasn't done before. The initialization of subtitle_header_size and extradata_size to 0 doesn't really matter the first time around, since they will be overwritten anyway as a result of using memcpy(), but the use of av_buffer_unref() does technically change the behavior compared to what it used to do. It is likely correct to do that the first time through, but I wonder, why not also do av_buffer_unref(>hw_device_ctx) and a bunch of the other things that avcodec_close() already does? The call to memcpy() will basically overwrite dest with the values in src, and besides the copies made of dest->codec and dest->priv_data earlier in the function, anything else that is overwritten could in theory leak. But, I guess that would have been a preexisting issue with this function and not really relevant for this patch. And this function is deprecated anyway. If there is no issue with the additional call to av_buffer_unref(), as mentioned above, this patch looks good. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true
On 4/22/2017 12:09 PM, Hendrik Leppkes wrote: On Sat, Apr 22, 2017 at 9:07 PM, Aaron Levinson <alevi...@aracnet.com> wrote: On 4/22/2017 2:16 AM, Clément Bœsch wrote: On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote: [...] diff --git a/doc/fate.texi b/doc/fate.texi index 7a96c25..f3b8c0c8 100644 --- a/doc/fate.texi +++ b/doc/fate.texi @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate @float NOTE Do not put a '~' character in the samples path to indicate a home directory. Because of shell nuances, this will cause FATE to fail. + +In addition, FATE will fail if files are checked out from git such +that @kbd{@key{CR}@key{LF}} is used for line endings. This will occur +on Windows when git is installed using the default options, because +that causes git's @var{core.autocrlf} setting to be set to +@option{true}. Make sure to set @var{core.autocrlf} to @option{input} +or @option{false}, or, in the case that the repository has already +been cloned, it is possible to get past this by executing the +following command in the top-level ffmpeg directory: @command{find +-name '*.h' -type f | xargs dos2unix}. @end float This is documented in doc/git-howto.texi, isn't it? Yes it is, but just because it can be found elsewhere in the documentation doesn't mean there isn't benefit to having a fallback elsewhere in the documentation. Can't we just make it point there, instead of explaining it again? OK, I'll do that. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true
On 4/22/2017 2:16 AM, Clément Bœsch wrote: On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote: [...] diff --git a/doc/fate.texi b/doc/fate.texi index 7a96c25..f3b8c0c8 100644 --- a/doc/fate.texi +++ b/doc/fate.texi @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate @float NOTE Do not put a '~' character in the samples path to indicate a home directory. Because of shell nuances, this will cause FATE to fail. + +In addition, FATE will fail if files are checked out from git such +that @kbd{@key{CR}@key{LF}} is used for line endings. This will occur +on Windows when git is installed using the default options, because +that causes git's @var{core.autocrlf} setting to be set to +@option{true}. Make sure to set @var{core.autocrlf} to @option{input} +or @option{false}, or, in the case that the repository has already +been cloned, it is possible to get past this by executing the +following command in the top-level ffmpeg directory: @command{find +-name '*.h' -type f | xargs dos2unix}. @end float This is documented in doc/git-howto.texi, isn't it? Yes it is, but just because it can be found elsewhere in the documentation doesn't mean there isn't benefit to having a fallback elsewhere in the documentation. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/7] avcodec: do not use AVFrame accessor
On 4/22/2017 2:27 AM, wm4 wrote: On Sat, 22 Apr 2017 16:04:22 +0700 Muhammad Faiz <mfc...@gmail.com> wrote: Signed-off-by: Muhammad Faiz <mfc...@gmail.com> --- +1 to patches 1-7. As long as the accessors only trivially access public fields, it's better (and less ugly) not to use accessors at all. Then why have the accessors at all if the fields are public? As far as I can tell, the benefit to using these accessors is that, if certain properties become internal in the future, or variable names change, or the structures become opaque, etc, the code will continue to work. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true
Note that in case it wasn't clear, I confirmed that this patch works by running make on Linux and examining both the generated text file and the generated HTML file in a Web browser. Aaron On 4/21/2017 4:48 PM, Aaron Levinson wrote: On 4/21/2017 4:17 PM, Lou Logan wrote: On Fri, 21 Apr 2017 16:05:16 -0700 Aaron Levinson <alevi...@aracnet.com> wrote: From 7ea6455cd52ffe561f94bf03f48c4c2cf61b33c5 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 21 Apr 2017 15:55:11 -0700 Subject: [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true Purpose: Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true. When CRLF is used for line endings, this causes the source FATE test, which uses check-source.sh, to fail. The issue has to do with the use of git grep, which doesn't appear to be able to handle CRLF line endings. Older versions of check-source.sh used grep directly, which seems to do a better job of handling CRLF line endings. Note: thanks to Hendrik Leppkes for suggestion that the issue with FATE failing was caused by CRLF line endings. --- doc/fate.texi | 9 + 1 file changed, 9 insertions(+) diff --git a/doc/fate.texi b/doc/fate.texi index 7a96c25..80fac0a 100644 --- a/doc/fate.texi +++ b/doc/fate.texi @@ -77,6 +77,15 @@ FATE_SAMPLES=fate-suite/ make fate @float NOTE Do not put a '~' character in the samples path to indicate a home directory. Because of shell nuances, this will cause FATE to fail. + +In addition, FATE will fail if files are checked out from git such +that CRLF is used for line endings. This will occur on Windows when +git is installed using the default options, because that causes git's +core.autocrlf setting to be set to 'true'. Make sure to set +core.autocrlf to 'input' or 'false', or, in the case that the +repository has already been cloned, it is possible to get past this by +executing the following command in the top-level ffmpeg directory: +@command{find -name '*.h' -type f | xargs dos2unix}. @end float This is a non-blocking minor comment, but it would be nice to add some additional markup to core.autocrlf and true/false. For a list see: <https://www.gnu.org/software/texinfo/manual/texinfo/html_node/Indicating.html#Indicating> How about the following? Thanks, Aaron Levinson From 707b61cfcc648eb228a157ce2c9ed0f61a2ba5ca Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 21 Apr 2017 16:45:28 -0700 Subject: [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true Purpose: Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true. When CRLF is used for line endings, this causes the source FATE test, which uses check-source.sh, to fail. The issue has to do with the use of git grep, which doesn't appear to be able to handle CRLF line endings. Older versions of check-source.sh used grep directly, which seems to do a better job of handling CRLF line endings. Note: thanks to Hendrik Leppkes for suggestion that the issue with FATE failing was caused by CRLF line endings. --- doc/fate.texi | 10 ++ 1 file changed, 10 insertions(+) diff --git a/doc/fate.texi b/doc/fate.texi index 7a96c25..f3b8c0c8 100644 --- a/doc/fate.texi +++ b/doc/fate.texi @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate @float NOTE Do not put a '~' character in the samples path to indicate a home directory. Because of shell nuances, this will cause FATE to fail. + +In addition, FATE will fail if files are checked out from git such +that @kbd{@key{CR}@key{LF}} is used for line endings. This will occur +on Windows when git is installed using the default options, because +that causes git's @var{core.autocrlf} setting to be set to +@option{true}. Make sure to set @var{core.autocrlf} to @option{input} +or @option{false}, or, in the case that the repository has already +been cloned, it is possible to get past this by executing the +following command in the top-level ffmpeg directory: @command{find +-name '*.h' -type f | xargs dos2unix}. @end float To use a custom wrapper to run the test, pass @option{--target-exec} to ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true
On 4/21/2017 4:17 PM, Lou Logan wrote: > On Fri, 21 Apr 2017 16:05:16 -0700 > Aaron Levinson <alevi...@aracnet.com> wrote: > >> From 7ea6455cd52ffe561f94bf03f48c4c2cf61b33c5 Mon Sep 17 00:00:00 2001 >> From: Aaron Levinson <alevi...@aracnet.com> >> Date: Fri, 21 Apr 2017 15:55:11 -0700 >> Subject: [PATCH] Added additional note to fate.texi to describe the >> importance >> of not checking out files from git with core.autocrlf set to true >> >> Purpose: Added additional note to fate.texi to describe the importance >> of not checking out files from git with core.autocrlf set to true. >> When CRLF is used for line endings, this causes the source FATE test, >> which uses check-source.sh, to fail. The issue has to do with the use >> of git grep, which doesn't appear to be able to handle CRLF line >> endings. Older versions of check-source.sh used grep directly, which >> seems to do a better job of handling CRLF line endings. >> >> Note: thanks to Hendrik Leppkes for suggestion that the issue with >> FATE failing was caused by CRLF line endings. >> --- >> doc/fate.texi | 9 + >> 1 file changed, 9 insertions(+) >> >> diff --git a/doc/fate.texi b/doc/fate.texi >> index 7a96c25..80fac0a 100644 >> --- a/doc/fate.texi >> +++ b/doc/fate.texi >> @@ -77,6 +77,15 @@ FATE_SAMPLES=fate-suite/ make fate >> @float NOTE >> Do not put a '~' character in the samples path to indicate a home >> directory. Because of shell nuances, this will cause FATE to fail. >> + >> +In addition, FATE will fail if files are checked out from git such >> +that CRLF is used for line endings. This will occur on Windows when >> +git is installed using the default options, because that causes git's >> +core.autocrlf setting to be set to 'true'. Make sure to set >> +core.autocrlf to 'input' or 'false', or, in the case that the >> +repository has already been cloned, it is possible to get past this by >> +executing the following command in the top-level ffmpeg directory: >> +@command{find -name '*.h' -type f | xargs dos2unix}. >> @end float > > This is a non-blocking minor comment, but it would be nice to add some > additional markup to core.autocrlf and true/false. For a list see: > > <https://www.gnu.org/software/texinfo/manual/texinfo/html_node/Indicating.html#Indicating> How about the following? Thanks, Aaron Levinson From 707b61cfcc648eb228a157ce2c9ed0f61a2ba5ca Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 21 Apr 2017 16:45:28 -0700 Subject: [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true Purpose: Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true. When CRLF is used for line endings, this causes the source FATE test, which uses check-source.sh, to fail. The issue has to do with the use of git grep, which doesn't appear to be able to handle CRLF line endings. Older versions of check-source.sh used grep directly, which seems to do a better job of handling CRLF line endings. Note: thanks to Hendrik Leppkes for suggestion that the issue with FATE failing was caused by CRLF line endings. --- doc/fate.texi | 10 ++ 1 file changed, 10 insertions(+) diff --git a/doc/fate.texi b/doc/fate.texi index 7a96c25..f3b8c0c8 100644 --- a/doc/fate.texi +++ b/doc/fate.texi @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate @float NOTE Do not put a '~' character in the samples path to indicate a home directory. Because of shell nuances, this will cause FATE to fail. + +In addition, FATE will fail if files are checked out from git such +that @kbd{@key{CR}@key{LF}} is used for line endings. This will occur +on Windows when git is installed using the default options, because +that causes git's @var{core.autocrlf} setting to be set to +@option{true}. Make sure to set @var{core.autocrlf} to @option{input} +or @option{false}, or, in the case that the repository has already +been cloned, it is possible to get past this by executing the +following command in the top-level ffmpeg directory: @command{find +-name '*.h' -type f | xargs dos2unix}. @end float To use a custom wrapper to run the test, pass @option{--target-exec} to -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true
From 7ea6455cd52ffe561f94bf03f48c4c2cf61b33c5 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 21 Apr 2017 15:55:11 -0700 Subject: [PATCH] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true Purpose: Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true. When CRLF is used for line endings, this causes the source FATE test, which uses check-source.sh, to fail. The issue has to do with the use of git grep, which doesn't appear to be able to handle CRLF line endings. Older versions of check-source.sh used grep directly, which seems to do a better job of handling CRLF line endings. Note: thanks to Hendrik Leppkes for suggestion that the issue with FATE failing was caused by CRLF line endings. --- doc/fate.texi | 9 + 1 file changed, 9 insertions(+) diff --git a/doc/fate.texi b/doc/fate.texi index 7a96c25..80fac0a 100644 --- a/doc/fate.texi +++ b/doc/fate.texi @@ -77,6 +77,15 @@ FATE_SAMPLES=fate-suite/ make fate @float NOTE Do not put a '~' character in the samples path to indicate a home directory. Because of shell nuances, this will cause FATE to fail. + +In addition, FATE will fail if files are checked out from git such +that CRLF is used for line endings. This will occur on Windows when +git is installed using the default options, because that causes git's +core.autocrlf setting to be set to 'true'. Make sure to set +core.autocrlf to 'input' or 'false', or, in the case that the +repository has already been cloned, it is possible to get past this by +executing the following command in the top-level ffmpeg directory: +@command{find -name '*.h' -type f | xargs dos2unix}. @end float To use a custom wrapper to run the test, pass @option{--target-exec} to -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1
On 4/19/2017 2:27 PM, Marton Balint wrote: On Mon, 17 Apr 2017, James Almer wrote: On 4/17/2017 5:39 AM, Clément Bœsch wrote: On Sun, Apr 16, 2017 at 05:20:02PM -0700, Aaron Levinson wrote: From 9e6a9e2b8d58f17c661a3f455e03c95587ec7b18 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Sun, 16 Apr 2017 17:13:31 -0700 Subject: [PATCH] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1 Purpose: libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1. This is only relevant when thread.h is included by C++ files. In this case, the relevant code is only defined if HAVE_PTHREADS is defined as 1. Use configure --assert-level=2 to do so. Note: Issue discovered as a result of Coverity build failure. Cause of build failure pinpointed by Hendrik Leppkes. Comments: -- libavutil/thread.h: Altered ASSERT_PTHREAD_NORET definition such that it uses av_make_error_string instead of av_err2str(). av_err2str() uses a "parenthesized type followed by an initializer list", which is apparently not valid C++. This issue started occurring because thread.h is now included by the DeckLink C++ files. The alteration does the equivalent of what av_err2str() does, but instead declares the character buffer as a local variable. --- libavutil/thread.h | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libavutil/thread.h b/libavutil/thread.h index 6e57447..f108e20 100644 --- a/libavutil/thread.h +++ b/libavutil/thread.h @@ -36,8 +36,11 @@ #define ASSERT_PTHREAD_NORET(func, ...) do {\ int ret = func(__VA_ARGS__);\ if (ret) { \ +char errbuf[AV_ERROR_MAX_STRING_SIZE] = ""; \ av_log(NULL, AV_LOG_FATAL, AV_STRINGIFY(func) \ - " failed with error: %s\n", av_err2str(AVERROR(ret))); \ + " failed with error: %s\n", \ + av_make_error_string(errbuf, AV_ERROR_MAX_STRING_SIZE, \ + AVERROR(ret))); \ abort();\ } \ } while (0) I don't like limiting ourselves in the common C code of the project because C++ is a bad and limited language. Can't you solve this by bumping the minimal requirement of C++ version? We're already using C++11 when available because of atomics on mediacodec. Also, just tried and it seems to fail even with C++14, so it just doesn't work with C++. We could instead just make these strict assert wrappers work only on C code by for example checking for defined(__cplusplus). I'd say let's apply the patch as is, that is the simplest solution. Regards, Marton I don't think most people care one way or the other--perhaps someone with push privileges could apply the patch? I assume that the Coverity builds are continuing to fail as a result of the issue that this patch addresses. Thanks, Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined
From 4f27e910aca6dae6642b4d73cf07fa0d6c4b1618 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Thu, 20 Apr 2017 23:20:20 -0700 Subject: [PATCH] Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined Purpose: Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined. If FF_API_LAVF_AVCTX is defined, AVStream::codec is set to an AVCodecContext object, and this wasn't being deallocated properly when the AVStream object was freed. While FF_API_LAVF_AVCTX has to do with deprecated functionality, in practice, it will always be defined for typical builds currently, since it is defined in libavformat\version.h if LIBAVFORMAT_VERSION_MAJOR is less than 58, and LIBAVFORMAT_VERSION_MAJOR is currently set to 57. Comments: -- libavformat/utils.c: Now using avcodec_free_context() to properly deallocate st->codec in free_stream() if FF_API_LAVF_AVCTX is defined. --- libavformat/utils.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index ba82a76..fbd8b58 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -4266,9 +4266,7 @@ static void free_stream(AVStream **pst) av_freep(>index_entries); #if FF_API_LAVF_AVCTX FF_DISABLE_DEPRECATION_WARNINGS -av_freep(>codec->extradata); -av_freep(>codec->subtitle_header); -av_freep(>codec); +avcodec_free_context(>codec); FF_ENABLE_DEPRECATION_WARNINGS #endif av_freep(>priv_data); -- 2.10.1.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] Added require fallback for libmfx in the case that pkg-config cannot find libmfx
On 4/14/2017 6:51 PM, Aaron Levinson wrote: From e0c73c054add0137901d0bf7a7893e42e7e566c8 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Fri, 14 Apr 2017 18:38:37 -0700 Subject: [PATCH] Added require fallback for libmfx in the case that pkg-config cannot find libmfx Purpose: Added require fallback for libmfx in the case that pkg-config cannot find libmfx. On Linux, most people likely get libmfx via https://github.com/lu-zero/mfx_dispatch , but on Windows, the most well-known way to get libmfx is via the Intel Media SDK, which provides a static build of libmfx.lib and also provides the source code for building libmfx yourself. If built this way, there are no pkg-config files to be found. The changes utilize a similar approach to that already done for libx264 in configure. Comments: -- configure: Altered enabled libmfx step to use use_pkg_config() instead of require_pkg_config(), and, if use_pkg_config() fails, it falls back to require(). Note that the reason that require() is passed -llibmfx as the last argument, instead of -lmfx, is the file name for the library produced from the Intel Media SDK starts with "libmfx". Apparently, the filename for the library produced via https://github.com/lu-zero/mfx_dispatch starts with "mfx". --- configure | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 3bea057..b20a0b4 100755 --- a/configure +++ b/configure @@ -5819,7 +5819,8 @@ enabled libgsm&& { for gsm_hdr in "gsm.h" "gsm/gsm.h"; do done || die "ERROR: libgsm not found"; } enabled libilbc && require libilbc ilbc.h WebRtcIlbcfix_InitDecode -lilbc enabled libkvazaar&& require_pkg_config "kvazaar >= 0.8.1" kvazaar.h kvz_api_get -enabled libmfx&& require_pkg_config libmfx "mfx/mfxvideo.h" MFXInit +enabled libmfx&& { use_pkg_config libmfx "mfx/mfxvideo.h" MFXInit || + { require libmfx "mfx/mfxvideo.h" MFXInit -llibmfx && warn "using libmfx without pkg-config"; } } enabled libmodplug&& require_pkg_config libmodplug libmodplug/modplug.h ModPlug_Load enabled libmp3lame&& require "libmp3lame >= 3.98.3" lame/lame.h lame_set_VBR_quality -lmp3lame enabled libnut&& require libnut libnut.h nut_demuxer_init -lnut Pinging this patch submission again. Thanks, Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1
On 4/17/2017 8:28 AM, wm4 wrote: On Mon, 17 Apr 2017 12:06:59 -0300 James Almer <jamr...@gmail.com> wrote: On 4/17/2017 5:39 AM, Clément Bœsch wrote: On Sun, Apr 16, 2017 at 05:20:02PM -0700, Aaron Levinson wrote: From 9e6a9e2b8d58f17c661a3f455e03c95587ec7b18 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Sun, 16 Apr 2017 17:13:31 -0700 Subject: [PATCH] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1 Purpose: libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1. This is only relevant when thread.h is included by C++ files. In this case, the relevant code is only defined if HAVE_PTHREADS is defined as 1. Use configure --assert-level=2 to do so. Note: Issue discovered as a result of Coverity build failure. Cause of build failure pinpointed by Hendrik Leppkes. Comments: -- libavutil/thread.h: Altered ASSERT_PTHREAD_NORET definition such that it uses av_make_error_string instead of av_err2str(). av_err2str() uses a "parenthesized type followed by an initializer list", which is apparently not valid C++. This issue started occurring because thread.h is now included by the DeckLink C++ files. The alteration does the equivalent of what av_err2str() does, but instead declares the character buffer as a local variable. --- libavutil/thread.h | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libavutil/thread.h b/libavutil/thread.h index 6e57447..f108e20 100644 --- a/libavutil/thread.h +++ b/libavutil/thread.h @@ -36,8 +36,11 @@ #define ASSERT_PTHREAD_NORET(func, ...) do {\ int ret = func(__VA_ARGS__);\ if (ret) { \ +char errbuf[AV_ERROR_MAX_STRING_SIZE] = ""; \ av_log(NULL, AV_LOG_FATAL, AV_STRINGIFY(func) \ - " failed with error: %s\n", av_err2str(AVERROR(ret))); \ + " failed with error: %s\n", \ + av_make_error_string(errbuf, AV_ERROR_MAX_STRING_SIZE, \ +AVERROR(ret))); \ abort();\ } \ } while (0) I don't like limiting ourselves in the common C code of the project because C++ is a bad and limited language. Can't you solve this by bumping the minimal requirement of C++ version? We're already using C++11 when available because of atomics on mediacodec. Also, just tried and it seems to fail even with C++14, so it just doesn't work with C++. We could instead just make these strict assert wrappers work only on C code by for example checking for defined(__cplusplus). Better solution: move all the code to a .c file. I've spent some time considering what would be involved in moving the relevant code into a .c file. thread.h is a header file that needs to be included to provide implementations of various pthread_ APIs on Windows and OS/2 without needing to link to pthread on those OS's. If pthread is available, it just includes pthread.h. So, it sort of acts like a portability layer. Providing it in the form of a .h file acts as a convenience. If the implementation were moved into a .c file, that tends to imply that it will reside in one of the ffmpeg libraries, likely libavutil. And that also implies that functions with the name pthread_create, etc, would be exported by libavutil, which is a bad idea. Instead, the right way to go is to provide a true threading portability layer with exported functions that start with, say, av_thread_. But, that's a decent project, and while I'm willing to undertake it, I would like to see some support for this endeavor first. However, there also seems to be some resistance to supporting C++ in ffmpeg. The DeckLink C++ files were contributed to ffmpeg in February 2014, over three years ago. While there is certainly no issue with using C-specific functionality in .c files, there is certainly an issue with doing so in header files that are intended to be used by any aspect of the project, whether in .c or .cpp files. thread.h is an example of a header file that should be suitable for use in either .c or .cpp files. The patch that I submitted accomplishes exactly that. Aaron Levinson ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1
From 9e6a9e2b8d58f17c661a3f455e03c95587ec7b18 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Sun, 16 Apr 2017 17:13:31 -0700 Subject: [PATCH] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1 Purpose: libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1. This is only relevant when thread.h is included by C++ files. In this case, the relevant code is only defined if HAVE_PTHREADS is defined as 1. Use configure --assert-level=2 to do so. Note: Issue discovered as a result of Coverity build failure. Cause of build failure pinpointed by Hendrik Leppkes. Comments: -- libavutil/thread.h: Altered ASSERT_PTHREAD_NORET definition such that it uses av_make_error_string instead of av_err2str(). av_err2str() uses a "parenthesized type followed by an initializer list", which is apparently not valid C++. This issue started occurring because thread.h is now included by the DeckLink C++ files. The alteration does the equivalent of what av_err2str() does, but instead declares the character buffer as a local variable. --- libavutil/thread.h | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libavutil/thread.h b/libavutil/thread.h index 6e57447..f108e20 100644 --- a/libavutil/thread.h +++ b/libavutil/thread.h @@ -36,8 +36,11 @@ #define ASSERT_PTHREAD_NORET(func, ...) do {\ int ret = func(__VA_ARGS__);\ if (ret) { \ +char errbuf[AV_ERROR_MAX_STRING_SIZE] = ""; \ av_log(NULL, AV_LOG_FATAL, AV_STRINGIFY(func) \ - " failed with error: %s\n", av_err2str(AVERROR(ret))); \ + " failed with error: %s\n", \ + av_make_error_string(errbuf, AV_ERROR_MAX_STRING_SIZE, \ +AVERROR(ret))); \ abort();\ } \ } while (0) -- 2.10.1.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1
I'll try that again with [PATCH] in the subject line. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1
From 9e6a9e2b8d58f17c661a3f455e03c95587ec7b18 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevi...@aracnet.com> Date: Sun, 16 Apr 2017 17:13:31 -0700 Subject: [PATCH] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1 Purpose: libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1. This is only relevant when thread.h is included by C++ files. In this case, the relevant code is only defined if HAVE_PTHREADS is defined as 1. Use configure --assert-level=2 to do so. Note: Issue discovered as a result of Coverity build failure. Cause of build failure pinpointed by Hendrik Leppkes. Comments: -- libavutil/thread.h: Altered ASSERT_PTHREAD_NORET definition such that it uses av_make_error_string instead of av_err2str(). av_err2str() uses a "parenthesized type followed by an initializer list", which is apparently not valid C++. This issue started occurring because thread.h is now included by the DeckLink C++ files. The alteration does the equivalent of what av_err2str() does, but instead declares the character buffer as a local variable. --- libavutil/thread.h | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libavutil/thread.h b/libavutil/thread.h index 6e57447..f108e20 100644 --- a/libavutil/thread.h +++ b/libavutil/thread.h @@ -36,8 +36,11 @@ #define ASSERT_PTHREAD_NORET(func, ...) do {\ int ret = func(__VA_ARGS__);\ if (ret) { \ +char errbuf[AV_ERROR_MAX_STRING_SIZE] = ""; \ av_log(NULL, AV_LOG_FATAL, AV_STRINGIFY(func) \ - " failed with error: %s\n", av_err2str(AVERROR(ret))); \ + " failed with error: %s\n", \ + av_make_error_string(errbuf, AV_ERROR_MAX_STRING_SIZE, \ +AVERROR(ret))); \ abort();\ } \ } while (0) -- 2.10.1.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avdevice/decklink: Removed pthread dependency
On 4/16/2017 4:25 PM, Aaron Levinson wrote: On 4/16/2017 3:31 PM, Hendrik Leppkes wrote: On Mon, Apr 17, 2017 at 12:26 AM, Aaron Levinson <alevi...@aracnet.com> wrote: On 4/16/2017 1:33 PM, Timo Rothenpieler wrote: Thanks, applied. Regards, Marton This seems to have broken the coverity builds: https://travis-ci.org/FFmpeg/FFmpeg-Coverity/builds/222597943#L1103 It was suggested on IRC by James Almer that "it seems to complain about ubitux's 'strict' pthread implementation (for assert levels above 0). Maybe it needs to be disabled for C++ sources." The real problem is in av_err2str, which uses a temporary array, which is either not valid in C++ (likely), or not supported by MSVC for reasons (although in contrast to C support, its C++ support is generally pretty good). But considering how little C++ code we have, its probably best to somehow avoid using it instead of trying to fix it. - Hendrik I don't think this has much to do with MSVC here, because coverity is doing a gcc/g++ build. I suspect that it is failing with any gcc/g++ build because of the patch. I'll work on this now. Aaron The build issue occurs with g++ and an assert level greater than 1, although it would also occur with MSVC++ if pthreads is enabled. I will submit a separate patch e-mail that fixes the build error. Aaron ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avdevice/decklink: Removed pthread dependency
On 4/16/2017 3:31 PM, Hendrik Leppkes wrote: On Mon, Apr 17, 2017 at 12:26 AM, Aaron Levinson <alevi...@aracnet.com> wrote: On 4/16/2017 1:33 PM, Timo Rothenpieler wrote: Thanks, applied. Regards, Marton This seems to have broken the coverity builds: https://travis-ci.org/FFmpeg/FFmpeg-Coverity/builds/222597943#L1103 It was suggested on IRC by James Almer that "it seems to complain about ubitux's 'strict' pthread implementation (for assert levels above 0). Maybe it needs to be disabled for C++ sources." The real problem is in av_err2str, which uses a temporary array, which is either not valid in C++ (likely), or not supported by MSVC for reasons (although in contrast to C support, its C++ support is generally pretty good). But considering how little C++ code we have, its probably best to somehow avoid using it instead of trying to fix it. - Hendrik I don't think this has much to do with MSVC here, because coverity is doing a gcc/g++ build. I suspect that it is failing with any gcc/g++ build because of the patch. I'll work on this now. Aaron ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Made minor changes to get the decklink avdevice code to build using Visual C++
On 4/15/2017 6:13 AM, Aaron Levinson wrote: > On 4/15/2017 4:19 AM, Marton Balint wrote: >> >> On Thu, 13 Apr 2017, Aaron Levinson wrote: >> >>> On 4/13/2017 1:23 PM, Hendrik Leppkes wrote: >> [...] >> >>> >>> >>> >>> From 00fdc9d15414a92a155eb7d1394bac3736dc9405 Mon Sep 17 00:00:00 2001 >>> From: Aaron Levinson <alevi...@aracnet.com> >>> Date: Thu, 13 Apr 2017 14:22:19 -0700 >>> Subject: [PATCH] Made minor changes to get the decklink avdevice code >>> to build using Visual C++. >>> >> >> Maybe it just me, but as I mentioned earlier, I don't like too verbose >> comments in the code, see below: >> >>> diff --git a/libavdevice/decklink_common.cpp >>> b/libavdevice/decklink_common.cpp >>> index f01fba9..523217c 100644 >>> --- a/libavdevice/decklink_common.cpp >>> +++ b/libavdevice/decklink_common.cpp >>> @@ -19,6 +19,15 @@ >>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>> 02110-1301 USA >>> */ >>> >>> +// Moved inclusion of internal.h here in order to get it to build >>> successfully >>> +// when using Visual C++ to build--otherwise, compilation errors result >>> +// due to winsock.h (which is included indirectly by DeckLinkAPI.h and >>> +// DeckLinkAPI_i.c) conflicting with winsock2.h, which is included by >>> +// internal.h. If winsock2.h is included first, then the conflict is >>> resolved. >> >> This can be as short as this: >> >> /* Include internal.h first to avoid conflict of winsock.h (used by >> * DeckLink) and winsock2.h (used by libavformat) in MSVC++ builds */ >> >> (for multiline comments I think /* */ is preferred) >> >> Although since you do this in multiple headers, maybe it is enough if >> you specify the reason in the commit message, and delete the comment >> from here entirely. > > I think it is a good idea to have a comment in the code, as then it is > front in center if someone in the future wonders why internal.h precedes > the other includes and decides to move it because it will look cleaner, > thereby breaking the MSVC build. Although, in that case, maybe it would > be preferable to have the same comment in each of the three places, as > opposed to just one. > > A shorter comment is fine, and your example comment explains things well > enough, although I tend to think that more information is better than > less for comments in code. From my perspective, "used by DeckLink" is a > bit vague, since technically, DeckLinkAPI.h and DeckLinkAPI_i.c would be > generated by the user if the actual Blackmagic DeckLink SDK were used > (these files are not actually supplied with the Blackmagic DeckLink > SDK), which is how I got DeckLinkAPI.h and DeckLinkAPI_c.h when I built > ffmpeg. Well, DeckLinkAPI.h is included in the Linux header files in > the Blackmagic DeckLink SDK, but the _i.c file is not, and on Windows, > neither file is provided. > > Regarding multi-line comments being wrapped in /* */ vs using // on each > line, it doesn't so much matter in this case, but I personally find // > more versatile because then it makes it easier to comment out blocks of > code. But, if that's the standard for the ffmpeg code base, then I'll > make that change. > >>> @@ -262,8 +265,18 @@ static int64_t >>> get_pkt_pts(IDeckLinkVideoInputFrame *videoFrame, >>> res = >>> videoFrame->GetHardwareReferenceTimestamp(time_base.den, _pts, >>> _duration); >>> break; >>> case PTS_SRC_WALLCLOCK: >>> -pts = av_rescale_q(wallclock, AV_TIME_BASE_Q, time_base); >>> +{ >>> +// doing the following rather than using AV_TIME_BASE_Q >>> because >>> +// AV_TIME_BASE_Q doesn't work when building with Visual >>> C++ >>> +// for C++ files (works for C files). When building C++ >>> files, >>> +// it results in compiler error C4576. At least, this is >>> the case >>> +// with Visual C++ 2015. >> >> And this is: >> >> // MSVC does not support compound literals like AV_TIME_BASE_Q in C++ >> code >> >>> +AVRational timebase; >>> + timebase.num = 1; >>> +timebase.den = AV_TIME_BASE; >>> +pts = av_rescale_q(wallclock, timebase, time_base); >>> br
Re: [FFmpeg-devel] [PATCH] avdevice/decklink: Removed pthread dependency
On 4/16/2017 1:33 PM, Timo Rothenpieler wrote: Thanks, applied. Regards, Marton This seems to have broken the coverity builds: https://travis-ci.org/FFmpeg/FFmpeg-Coverity/builds/222597943#L1103 It was suggested on IRC by James Almer that "it seems to complain about ubitux's 'strict' pthread implementation (for assert levels above 0). Maybe it needs to be disabled for C++ sources." Aaron ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel