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

[Gyan Doshi]

On 2024-02-16 01:56 am, Kieran Kunhya wrote:
> On Thu, 15 Feb 2024 at 16:48, Gyan Doshi <ffmpeg at gyani.pro> wrote:
>
>>
>> On 2024-02-15 09:40 pm, Anton Khirnov wrote:
>>> Quoting Gyan Doshi (2024-02-15 13:31:59)
>>>> On 2024-02-15 04:17 pm, Anton Khirnov wrote:
>>>>> Hi,
>>>>> sorry for the delay, I've been busy fixing things for the release
>>>>> Quoting Gyan Doshi via ffmpeg-devel (2024-01-29 05:00:33)
>>>>>> On 2024-01-28 04:24 pm, Anton Khirnov wrote:
>>>>>>>> a) it would mean essentially inlining this decoder in the demuxer.
>>>>>>> Why is that a problem? This decoder seems like it shouldn't be a
>>>>>>> decoder.
>>>>>>>
>>>>>>> I agree with Andreas that this seems like it's a demuxer pretending
>> to
>>>>>>> be a decoder.
>>>>>> This module transforms the entire raw payload data to generate its
>>>>>> output, even if the syntax is simple which
>>>>>> essentially makes it a de-coder. The de-multiplexer aspect of multiple
>>>>>> streams is an academic possibility allowed
>>>>>> by the standard but not seen in any sample which makes me suspect it's
>>>>>> used for carriage between broadcast
>>>>>> facilities rather than something ever sent to an OTT provider, let
>> alone
>>>>>> an end user.
>>>>> If it dynamically generates nested decoders, then it's not a proper
>>>>> codec in our model. It should be either a part of the demuxer, or a
>>>>> bitstream filter (possibly inserted automatically by the demuxer).
>>>> s302m is a hybrid creature and does not slot cleanly into any role. So
>>>> there is no theoretically proper place for this component - any choice
>>>> is a least-out-of-place accommodation.
>>>>
>>>> But it is much more out of place inside a demuxer. Analyzing packet
>>>> payload and then manipulating that entire payload is much closer to a
>>>> decoding role than data chunk extraction for packetization.
>>> I don't see why specifically this property should be the one
>>> distinguishing demuxers from decoders, it sounds pretty arbitrary to me.
>>> Many demuxers apply transformations of far higher complexity to the
>>> bytestream before exporting it, e.g. in matroska the packet data may be
>>> compressed, laced, etc.
>>>
>>>> And the stream extracted from the container is meant to be SMPTE ST
>>>> 302 not PCM* or Dolby-E/AC-3..etc,
>>> "meant to be"? By whom?
>>>
>>> The point of libavformat is to abstract away the differences between
>>> containers as much as is reasonably feasible, and export the data in the
>>> format most useful to the caller for decoding or other processing.
>>>
>>>> which will both misrepresent what the container carries
>>> Why should the caller care?
>>>
>>>> and possibly discard S-ADM metadata, if present, in the packet.
>>> Why could that not be exported as side data?
>>>
>>>> A bsf in principle would work but in practice, can't as Andreas
>>>> clarified that bsfs can't set or alter codec_id after init. And
>>>> resetting the codec id requires packet inspection.
>>> There are two possibilities then - either extend the BSF API to support
>>> multiple output streams, or implement it inside libavformat as a
>>> post-demuxer hook called in the same place as parsing.
>>>
>>>> Nested decoders are used without issue in components like imm5 or ftr
>>>> (upto 64 nested decoders!) among others. There's no breaking of new
>>>> ground here.
>>> Nested decoders are certainly not "without issue" - they are a constant
>>> source of issues, since implementing nesting properly is very tricky. I
>>> am fairly sure most nested decoders we have are subtly broken in various
>>> ways.
>> This patch facilitates a certain productive use of ffmpeg with respect
>> to processing of live inputs that wasn't possible earlier,
>> and which currently is being used successfully by multiple people over
>> the past few weeks.
>> It applies a processing model already implemented in multiple other
>> decoders for a number of years. I haven't seen many reports
>> of issues with them. And surely something being 'a constant source of
>> issues' would be a lot more than 'subtly broken' as you describe
>> them.You're the only one who has objected on architectural grounds and
>> this looks to be a fundamental disagreement.
>> If you are blocking this patch, do acknowledge here within 24 hours and
>> we can send this to the TC else I'll push it after that period.
>>
> Dolby E can exist in other containers apart from S302M. Raising the TC is
> premature.

This patch only concerns s302m and sets up a framework for non-PCM 
decode. Dolby-E is incidental.

Regards,
Gyan