[FFmpeg-devel] [TC] Technical decision on S302M non-PCM decoding

[Niklas Haas]

On Sat, 17 Feb 2024 17:15:43 +0530 Gyan Doshi <ffmpeg at gyani.pro> wrote:
> Issue:
> 
> Patch: avcodec/s302m: enable non-PCM decoding
> URL: 
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240127103854.9971-1-ffmpeg@gyani.pro/
> 
> The issue needing resolution is whether the patch should be added to the 
> existing s302m decoder or should that decoder
> be removed and all old and new patched features inlined into the mpeg-ts 
> demuxer.
> 
> Summary:
> 
> SMPTE ST 302 specifies for carriage of LPCM media in MPEG-TS. SMPTE ST 
> 337 enables this for non-PCM codecs. The payload
> has a custom layout so it can't be directly processed hence lavc/s302m 
> decodes the packet data to yield LPCM media. But
> it can only deal at present with LPCM payloads, meaning that non-PCM 
> payloads need to be exported to a raw bytestream
> format and then decoded in a 2nd step, which prohibits direct 
> transcoding of live/streaming inputs. This patch corrects
> the identification process for non-PCM payloads, reformats the payload 
> and then carries out in-place decoding by calling
> a nested decoder similar to the ftr or imm5 decoders in lavc.
> 
> In the v1 patch review, Andreas, in response to the proposed doc entry 
> describing the feature capability of multiple
> or differing payloads in a s302m stream, suggested[1] that s302m should 
> be a bitstream filter instead, but I did not
> see that as an actionable suggestion as he immediately listed the bsf 
> limitations preventing the possibility. I also
> had not seen an actual sample of s302m with multiple embedded streams. 
> Kieran also observed[2] that he had not seen
> such a stream in the wild. So the added features of this patch, wherever 
> they are ultimately located, shall not yield
> more than one media stream. Anton suggested[3] that the decoder should 
> instead be a demuxer. I saw no other objections
> to the architecture of the patch.
> 
> I posted the v2 patch, incorporating some changes suggested by Andreas, 
> 4 days later. This had gone uncommented for
> over two weeks when I posted a notice stating an intention to push. 
> Anton posted[4] a new objection that "If it
> dynamically generates nested decoders, then it's not a proper codec in 
> our model". This new objection is not connected
> to multiple streams but only to a codec 'model' that I don't see 
> described anywhere and which contradicts the
> implementations of multiple decoders with a nested decoder, including 
> the ftr and imm5 decoders, which are most similar
> in design to the patched s302m decoder. Anton later on mentioned[5] that 
> nested decoders are "a constant source of
> issues". However, I didn't find anything on trac reporting an issue with 
> the nested decoders of ftr and imm5 nor
> anything on ffmpeg-devel-ml or ffmpeg-user-ml. Nothing in their commit 
> history either points to architectural bugs.
> These decoders have been around for 6 years among themselves. The 
> testing of the patched s302m decoder over the past
> month by myself, an OTT provider and others shows no issues either. 
> Finally, Anton speculates[6] that the burden of
> fixes will likely fall upon him. In none of his objections, till the 
> time of writing, did I see specific concerns with
> the patch.
> 
> There are some limitations in shifting this decoder wholesale to inside 
> the MPEG-TS demuxer. A s302m stream may contain
> some non-media payload accompanying non-PCM media i.e. S-ADM metadata. 
> At present, I have neither the samples nor the
> specification needed in order to locate and extract or parse this 
> metadata. Formatting the payload data inside the
> demuxer will lead to irrevocable loss of such metadata if present. 
> However, a decoder patch allows simultaneuous output
> of both a decoded stream alongside a copied stream. The end-user can 
> then do with the raw data whatever they wish.
> 
> Ultimately, s302m is specified an an elementary stream inside a MPEG-TS 
> container. Its internal handling is better left
> to a dedicated module like a decoder. A bitstream filter might be a 
> better fit if s302m streams with multiple media
> payloads ever start appearing - none have, so far - but for single media 
> payloads, a decoder remains the best place.
> 
> Regards,
> Gyan
> 
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-January/320119.html
> [2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-January/320321.html
> [3]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-January/320258.html
> [4]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/321514.html
> [5]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/321523.html
> [6]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/321539.html
> 

Hi,

This is in regards to the patch "avcodec/s302m: enable non-PCM decoding"
by Gyan Doshi.
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240127103854.9971-1-ffmpeg@gyani.pro/

The TC has voted on the matter, with all 5 voting members unanimously
voting against the patch.

The opposition to the patch was based on the opinion that this
implementation of S302M should be handled by libavformat, not
libavcodec. The central argument focused on the ways in which these
formats can interact. In particular, S302M is always tied to MPEG-TS,
but Dolby E can also exist in other containers, so exposing Dolby E as
the the intermediary (AV_CODEC_ID) unlocks the full range of possible
operations (e.g. S302M -> Dolby E -> MXF remux).

Regarding Gyan Doshi's main objection that doing so would possibly
delete S302M metadata (e.g. S-ADM), it was observed that if the need for
such metadata truly arises, it should either be parsed by FFmpeg or
attached as an opaque side data blob (only if the former is impossible).

Therefore, the matter has been decided, rejecting the patch at hand.

Regards,
The FFmpeg Technical Committee
- Jan Ekström
- Niklas Haas
- Anton Khirnov
- Michael Niedermayer
- Martin Storsjö
- Mark Thompson