[FFmpeg-devel] [PATCH 1/2] avcodec/s302m: enable non-PCM decoding

[Gyan Doshi]

On 2024-01-25 10:29 am, Andreas Rheinhardt wrote:
> Gyan Doshi:
>> Set up framework for non-PCM decoding in-place and
>> add support for Dolby-E decoding.
>>
>> Useful for direct transcoding of non-PCM audio in live inputs.
>> ---
>>   configure          |   1 +
>>   doc/decoders.texi  |  40 +++
>>   libavcodec/s302m.c | 609 +++++++++++++++++++++++++++++++++++++--------
>>   3 files changed, 543 insertions(+), 107 deletions(-)
>>
>> diff --git a/configure b/configure
>> index c8ae0a061d..8db3fa3f4b 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2979,6 +2979,7 @@ rv20_decoder_select="h263_decoder"
>>   rv20_encoder_select="h263_encoder"
>>   rv30_decoder_select="golomb h264pred h264qpel mpegvideodec rv34dsp"
>>   rv40_decoder_select="golomb h264pred h264qpel mpegvideodec rv34dsp"
>> +s302m_decoder_select="dolby_e_decoder"
>>   screenpresso_decoder_deps="zlib"
>>   shorten_decoder_select="bswapdsp"
>>   sipr_decoder_select="lsp"
>> diff --git a/doc/decoders.texi b/doc/decoders.texi
>> index 293c82c2ba..9f85c876bf 100644
>> --- a/doc/decoders.texi
>> +++ b/doc/decoders.texi
>> @@ -347,6 +347,46 @@ configuration. You need to explicitly configure the build with
>>   An FFmpeg native decoder for Opus exists, so users can decode Opus
>>   without this library.
>>   
>> + at section s302m
>> +
>> +SMPTE ST 302 decoder.
>> +
>> +SMPTE ST 302 is a method for storing AES3 data format within an MPEG Transport
>> +Stream. AES3 streams can contain LPCM streams of 2, 4, 6 or 8 channels with a
>> +bit depth of 16, 20 or 24-bits at a sample rate of 48 kHz.
>> +They can also contain non-PCM codec streams such as AC-3 or Dolby-E.
>> +
> This sounds like we should add bitstream filters to extract the proper
> underlying streams instead.
> (I see only two problems with this approach: The BSF API needs to set
> the CodecID of the output during init, but at this point no packet has
> reached the BSF to determine it. And changing codec IDs mid-stream is
> also not supported.)

In theory, this decoder shouldn't exist, as it is just a carrier, 
whether of LPCM or non-PCM.
FFmpeg architecture also imposes a fundamental limitation in that one 
s302m stream may
carry multiple payload streams and we support only one decoding context 
per input stream
neither can a bsf spawn streams (not sure). So proper, full support 
seems not possible.

[...]

>> +
>> +    ret = init_get_bits8(&gb, buf, buf_size);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    aes_frm_size = (s->bits + 4) * 2 / 8;
>> +    if (buf_size < aes_frm_size * 2)  // not enough to contain data_type & length_code
>> +        return AVERROR_INVALIDDATA;
>> +
>> +    state = get_bits64(&gb, aes_frm_size * 8);
>> +
>> +    while (!IS_NONPCMSYNC(s->bits,state) && (get_bits_left(&gb) >= 8))
>> +        state = (state << 8) | get_bits(&gb, 8);
> Reading byte-aligned data with a GetBit context is very suboptimal.

What is the performance difference vs. uint8 pointers?
Note that if stream is LPCM or non-decodable non-PCM, this isn't called 
again. If it is Dolby-E, the data traversed can typically be measured in 
the dozens of bytes. And further on, I do read and skip some 
non-byte-aligned lengths.

[...]

>> +
>> +            if (s->non_pcm_dec)
>> +                for (int i = 0; i < 4; i++)
>> +                    *p++ = b[i];
>> +            else {
>> +                *f16++ = (b[0] << 8) |
>> +                         (b[1]     ) ;
> AV_RB16(b)

Ok.

[...]
>> +
>> +        for (int ch = 0; ch < s->frame->ch_layout.nb_channels; ch++)
>> +            memcpy(frame->extended_data[ch], s->frame->extended_data[ch],
>> +                   av_get_bytes_per_sample(s->non_pcm_ctx->sample_fmt) * s->frame->nb_samples);
> Would you please explain to me why this extra frame s->frame exists at
> all? (Is it just the assert due to the missing FrameDecodeData? If so,
> then this should be changed instead.)

Yes, that assert was triggered. I haven't looked into the ramifications 
of altering decode_receive_frame_internal and it's out of scope for this 
patch.
If you feel strongly about it, I invite you to change that code and I'll 
update this patch accordingly.

[...]

>> +static av_cold int s302m_close(AVCodecContext *avctx)
>> +{
>> +    S302Context *s = avctx->priv_data;
>> +
>> +    avcodec_free_context(&s->non_pcm_ctx);
>> +    av_packet_free(&s->packet);
>> +    av_frame_free(&s->frame);
>> +    av_dict_free(&s->non_pcm_opts);
> non_pcm_opts is an av_opt-enabled field and is therefore freed generically.

Will remove.

Regards,
Gyan