| Message ID | 8C6A6F6D-6526-4270-9EE9-3DA9AE69B6D2@dericed.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Oct 17, 2017 at 11:12:46 -0400, Dave Rice wrote: > Thanks for pointing me in the right direction. I am attaching an > updated patch below; however, after removing my 16/32 and using the > enumeration method (with the patch below), it accepts any value > between AUDIO_BITDEPTH_LOWEST and AUDIO_BITDEPTH_NB-1 (16 and 32). > When I enter any value from 17 through 31 then the output is still in > pcm_s16le, however the resulting audio is slow and choppy (I'm > guessing from the pkt.size line). Am I missing some method to say > that the enumeration list is restrictive? Yes, that was exactly the oversight I had. The const'ified option now takes those strings, but also numbers within the range. So the option value "16" will be either "16" (string) or 16 (or integer), "17" will be interpreted as integer 17 (and still valid thanks to the range), and so on. My bad. So I guess it's stupid to use a string enum to represent a number value. You could properly restrict its use to the enumerated values if they were subsequent (i.e. 0, 1), and perhaps strings with letters for the enumerated options. But that's totally unintuitive. It would result in (auto doc): -audio_depth <int> .D...... audio bitdepth (from 0 to 1) (default 16bits) 16bits .D...... 32bits .D...... and you could use it with either -audio_depth 0 # 16 bits -audio_depth 1 # 32 bits -audio_depth 16bits # 16 bits -audio_depth 32bits # 16 bits but not -audio_depth 16 Horrible! Your original version was most intuitive, IMHO. Sorry for explaining the concept. ;-) It fits well for anything representable by strings, not numbers. Moritz
On 10/17/17, Moritz Barsnick <barsnick@gmx.net> wrote: > On Tue, Oct 17, 2017 at 11:12:46 -0400, Dave Rice wrote: > >> Thanks for pointing me in the right direction. I am attaching an >> updated patch below; however, after removing my 16/32 and using the >> enumeration method (with the patch below), it accepts any value >> between AUDIO_BITDEPTH_LOWEST and AUDIO_BITDEPTH_NB-1 (16 and 32). >> When I enter any value from 17 through 31 then the output is still in >> pcm_s16le, however the resulting audio is slow and choppy (I'm >> guessing from the pkt.size line). Am I missing some method to say >> that the enumeration list is restrictive? > > Yes, that was exactly the oversight I had. The const'ified option now > takes those strings, but also numbers within the range. So the option > value "16" will be either "16" (string) or 16 (or integer), "17" will > be interpreted as integer 17 (and still valid thanks to the range), and > so on. My bad. > > So I guess it's stupid to use a string enum to represent a number > value. You could properly restrict its use to the enumerated values if > they were subsequent (i.e. 0, 1), and perhaps strings with letters for > the enumerated options. But that's totally unintuitive. It would result > in (auto doc): > > -audio_depth <int> .D...... audio bitdepth (from 0 to 1) > (default 16bits) > 16bits .D...... > 32bits .D...... > > and you could use it with either > -audio_depth 0 # 16 bits > -audio_depth 1 # 32 bits > -audio_depth 16bits # 16 bits > -audio_depth 32bits # 16 bits > but not > -audio_depth 16 > > Horrible! > > Your original version was most intuitive, IMHO. Sorry for explaining > the concept. ;-) It fits well for anything representable by strings, > not numbers. Hmm, first patch might be enough.
> On Oct 17, 2017, at 11:48 AM, Paul B Mahol <onemda@gmail.com> wrote: > > On 10/17/17, Moritz Barsnick <barsnick@gmx.net> wrote: >> On Tue, Oct 17, 2017 at 11:12:46 -0400, Dave Rice wrote: >> >>> Thanks for pointing me in the right direction. I am attaching an >>> updated patch below; however, after removing my 16/32 and using the >>> enumeration method (with the patch below), it accepts any value >>> between AUDIO_BITDEPTH_LOWEST and AUDIO_BITDEPTH_NB-1 (16 and 32). >>> When I enter any value from 17 through 31 then the output is still in >>> pcm_s16le, however the resulting audio is slow and choppy (I'm >>> guessing from the pkt.size line). Am I missing some method to say >>> that the enumeration list is restrictive? >> >> Yes, that was exactly the oversight I had. The const'ified option now >> takes those strings, but also numbers within the range. So the option >> value "16" will be either "16" (string) or 16 (or integer), "17" will >> be interpreted as integer 17 (and still valid thanks to the range), and >> so on. My bad. >> >> So I guess it's stupid to use a string enum to represent a number >> value. You could properly restrict its use to the enumerated values if >> they were subsequent (i.e. 0, 1), and perhaps strings with letters for >> the enumerated options. But that's totally unintuitive. It would result >> in (auto doc): >> >> -audio_depth <int> .D...... audio bitdepth (from 0 to 1) >> (default 16bits) >> 16bits .D...... >> 32bits .D...... >> >> and you could use it with either >> -audio_depth 0 # 16 bits >> -audio_depth 1 # 32 bits >> -audio_depth 16bits # 16 bits >> -audio_depth 32bits # 16 bits >> but not >> -audio_depth 16 >> >> Horrible! >> >> Your original version was most intuitive, IMHO. Sorry for explaining >> the concept. ;-) It fits well for anything representable by strings, >> not numbers. > > Hmm, first patch might be enough. Sounds good to me. Unless anyone prefers "-audio_depth thirtytwo" :-D Dave Rice
On 2017-10-17 09:10, Dave Rice wrote: >>> >>> -audio_depth <int> .D...... audio bitdepth (from 0 to >>> 1) >>> (default 16bits) >>> 16bits .D...... >> >> Hmm, first patch might be enough. > > Sounds good to me. Unless anyone prefers "-audio_depth thirtytwo" :-D > Dave Rice > Yeah that works.. so if they have any other depths, can go 0, 1 or 2 (2=some new bit depth yet to be created) And for clarification: yes 32-bits (PCM_S32LE) -- I was just pointing out the ADC/DAC's are 24-bit (8-bits padded). --Doug (dx9s)
> On Oct 17, 2017, at 3:30 PM, Douglas Marsh <ffmpeg@dx9s.net> wrote: > > On 2017-10-17 09:10, Dave Rice wrote: > >>>> -audio_depth <int> .D...... audio bitdepth (from 0 to 1) >>>> (default 16bits) >>>> 16bits .D...... > >>> Hmm, first patch might be enough. >> Sounds good to me. Unless anyone prefers "-audio_depth thirtytwo" :-D >> Dave Rice > > Yeah that works.. so if they have any other depths, can go 0, 1 or 2 (2=some new bit depth yet to be created) > > And for clarification: yes 32-bits (PCM_S32LE) -- I was just pointing out the ADC/DAC's are 24-bit (8-bits padded). I was suggesting `-audio_depth thirtytwo` in jest. IMHO assigning enumerated index numbers to purely numerical values will be confusing. For instance if in the future 12 bit is added then 24 bit, we'll have -audio_depth <int> .D...... audio bitdepth (from 0 to 3) (default 16bits) 16bits [0] 32bits [1] 12bits [2] 24bits [3] The alternative patch in the "decklink 24/32 bit question" thread, changes the default behavior of the decklink input which I think should be avoided. I agree with Moritz that the first patch of this thread (https://patchwork.ffmpeg.org/patch/5588/) is the best option. Also the method used in the patch to validate a limited non-consecutive range of values is already in used elsewhere in the device for the channel count at https://github.com/FFmpeg/FFmpeg/blob/278588cd0bf788df0194f74e62745f68559616f9/libavdevice/decklink_dec.cpp#L859-L868. Best Regards, Dave Rice
diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h index 6b2525fb53..b6acb01bb9 100644 --- a/libavdevice/decklink_common.h +++ b/libavdevice/decklink_common.h @@ -97,6 +97,7 @@ struct decklink_ctx { int frames_buffer_available_spots; int channels; + int audio_depth; }; typedef enum { DIRECTION_IN, DIRECTION_OUT} decklink_direction_t; diff --git a/libavdevice/decklink_common_c.h b/libavdevice/decklink_common_c.h index 5616ab32f9..cf534c1413 100644 --- a/libavdevice/decklink_common_c.h +++ b/libavdevice/decklink_common_c.h @@ -30,6 +30,13 @@ typedef enum DecklinkPtsSource { PTS_SRC_WALLCLOCK = 4, } DecklinkPtsSource; +typedef enum audio_bitdepths { + AUDIO_BITDEPTH_LOWEST = 16, + AUDIO_BITDEPTH_16 = 16, + AUDIO_BITDEPTH_32 = 32, + AUDIO_BITDEPTH_NB +} audio_bitdepths; + struct decklink_cctx { const AVClass *cclass; @@ -42,6 +49,7 @@ struct decklink_cctx { double preroll; int v210; int audio_channels; + int audio_depth; int duplex_mode; DecklinkPtsSource audio_pts_source; DecklinkPtsSource video_pts_source; diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp index d9ac01ac91..4ecd02b030 100644 --- a/libavdevice/decklink_dec.cpp +++ b/libavdevice/decklink_dec.cpp @@ -771,7 +771,7 @@ HRESULT decklink_input_callback::VideoInputFrameArrived( av_init_packet(&pkt); //hack among hacks - pkt.size = audioFrame->GetSampleFrameCount() * ctx->audio_st->codecpar->channels * (16 / 8); + pkt.size = audioFrame->GetSampleFrameCount() * ctx->audio_st->codecpar->channels * (ctx->audio_depth / 8); audioFrame->GetBytes(&audioFrameBytes); audioFrame->GetPacketTime(&audio_pts, ctx->audio_st->time_base.den); pkt.pts = get_pkt_pts(videoFrame, audioFrame, wallclock, ctx->audio_pts_source, ctx->audio_st->time_base, &initial_audio_pts); @@ -854,6 +854,7 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) ctx->audio_pts_source = cctx->audio_pts_source; ctx->video_pts_source = cctx->video_pts_source; ctx->draw_bars = cctx->draw_bars; + ctx->audio_depth = cctx->audio_depth; cctx->ctx = ctx; /* Check audio channel option for valid values: 2, 8 or 16 */ @@ -930,7 +931,7 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) goto error; } st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; - st->codecpar->codec_id = AV_CODEC_ID_PCM_S16LE; + 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, 1000000); /* 64 bits pts in us */ @@ -1021,7 +1022,7 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx) } av_log(avctx, AV_LOG_VERBOSE, "Using %d input audio channels\n", ctx->audio_st->codecpar->channels); - result = ctx->dli->EnableAudioInput(bmdAudioSampleRate48kHz, bmdAudioSampleType16bitInteger, ctx->audio_st->codecpar->channels); + result = ctx->dli->EnableAudioInput(bmdAudioSampleRate48kHz, cctx->audio_depth == 32 ? bmdAudioSampleType32bitInteger : bmdAudioSampleType16bitInteger, ctx->audio_st->codecpar->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 1127d23ada..e6e1deca65 100644 --- a/libavdevice/decklink_dec_c.c +++ b/libavdevice/decklink_dec_c.c @@ -72,6 +72,9 @@ static const AVOption options[] = { { "wallclock", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = PTS_SRC_WALLCLOCK}, 0, 0, DEC, "pts_source"}, { "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", OFFSET(audio_depth), AV_OPT_TYPE_INT, { .i64 = AUDIO_BITDEPTH_16 }, AUDIO_BITDEPTH_LOWEST, AUDIO_BITDEPTH_NB-1, DEC, "audio_depth" }, + { "16", "16 bits", 0, AV_OPT_TYPE_CONST, { .i64 = AUDIO_BITDEPTH_16 }, 0, 0, DEC, "audio_depth" }, + { "32", "32 bits", 0, AV_OPT_TYPE_CONST, { .i64 = AUDIO_BITDEPTH_32 }, 0, 0, DEC, "audio_depth" }, { NULL }, };