Re: [FFmpeg-devel] [PATCH] Revert "avformat/mpegts: update stream info when PMT ES stream_type changes"

[Michael Niedermayer via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4528 bytes --]

HI Kacper

On Wed, Aug 13, 2025 at 02:42:13AM +0200, Kacper Michajlow wrote:
> On Sat, 14 Jun 2025 at 22:47, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Fri, Jun 13, 2025 at 04:06:31PM -0600, Pavel Koshevoy wrote:
> > > On Fri, Jun 13, 2025 at 2:46 PM Michael Niedermayer <michael@niedermayer.cc>
> > > wrote:
> > >
> > > > On Mon, Jun 09, 2025 at 02:25:51PM +0200, Michael Niedermayer wrote:
> > > > > This fixes mixing up contexts, use of uninitialized data and crashes.
> > > > > More specifically:
> > > > >
> > > > > ==1001752== Conditional jump or move depends on uninitialised value(s)
> > > > > ==1001752==    at 0xA9ED82: avpriv_h264_has_num_reorder_frames
> > > > (h264dec.c:64)
> > > > > ==1001752==    by 0x668C7E: has_decode_delay_been_guessed (demux.c:757)
> > > > > ==1001752==    by 0x66AB13: compute_pkt_fields (demux.c:1137)
> > > > > ==1001752==    by 0x66B2E9: parse_packet (demux.c:1265)
> > > > > ==1001752==    by 0x66BD84: read_frame_internal (demux.c:1449)
> > > > > ==1001752==    by 0x67085B: avformat_find_stream_info (demux.c:2692)
> > > > > ==1001752==    by 0x25157C: ifile_open (ffmpeg_demux.c:1814)
> > > > > ==1001752==    by 0x272B15: open_files (ffmpeg_opt.c:1366)
> > > > > ==1001752==    by 0x272D85: ffmpeg_parse_options (ffmpeg_opt.c:1415)
> > > > > ==1001752==    by 0x2925C9: main (ffmpeg.c:991)
> > > > > ==1001752==  Uninitialised value was created by a heap allocation
> > > > > ==1001752==    at 0x483E0F0: memalign (in
> > > > /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > > > ==1001752==    by 0x483E212: posix_memalign (in
> > > > /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > > > ==1001752==    by 0x14882CE: av_malloc (mem.c:107)
> > > > > ==1001752==    by 0x1463785: av_buffer_alloc (buffer.c:82)
> > > > > ==1001752==    by 0x146423F: pool_alloc_buffer (buffer.c:369)
> > > > > ==1001752==    by 0x14643C4: av_buffer_pool_get (buffer.c:407)
> > > > > ==1001752==    by 0x752C4B: buffer_pool_get (mpegts.c:1142)
> > > > > ==1001752==    by 0x7538F2: mpegts_push_data (mpegts.c:1407)
> > > > > ==1001752==    by 0x758893: handle_packet (mpegts.c:2909)
> > > > > ==1001752==    by 0x758E90: handle_packets (mpegts.c:3048)
> > > > > ==1001752==    by 0x759B1D: mpegts_read_packet (mpegts.c:3290)
> > > > > ==1001752==    by 0x6687A3: ff_read_packet (demux.c:649)
> > > > > ==1001752==    by 0x66B594: read_frame_internal (demux.c:1346)
> > > > > ==1001752==    by 0x67085B: avformat_find_stream_info (demux.c:2692)
> > > > > ==1001752==    by 0x25157C: ifile_open (ffmpeg_demux.c:1814)
> > > > > ==1001752==    by 0x272B15: open_files (ffmpeg_opt.c:1366)
> > > > > ==1001752==    by 0x272D85: ffmpeg_parse_options (ffmpeg_opt.c:1415)
> > > > > ==1001752==    by 0x2925C9: main (ffmpeg.c:991)
> > > > >
> > > > > Found-by: Alexander A. Shvedov <shvedov@gmx.com>
> > > > > CC: Pavel Koshevoy <pkoshevoy@gmail.com>
> > > > > This reverts commit 0021484d05f9b0f032fa319399de6e24eea0c04f.
> > > > > ---
> > > > >  libavformat/mpegts.c    | 4 +---
> > > > >  tests/ref/fate/ts-demux | 4 ++--
> > > > >  2 files changed, 3 insertions(+), 5 deletions(-)
> > > >
> > > > does anyone object to this ?
> > > >
> > > > Pavel, are you ok with this being reverted or do you want to submit a
> > > > patch that puts this behind some runtime flag ?
> > > >
> > >
> > > yeah, you can revert it ... Once I have the runtime flag implemented I can
> > > resubmit it all together.
> >
> > ok will revert
> 
> Is this still a plan?

I was a bit undecided on this, i may have overestimated the problems this
would cause. And nothing major broke, and noone else spoke up ...
So I kind of conditionally droped this patch until someone else said something or
something broke ...


> I also see a memory leak after this change.
> OSS-Fuzz testcase 5851268507828224.
> 
> Additionally, it looks weird to skip `mpegts_set_stream_info()` on
> this pes, but a few lines below still use it in
> `av_program_add_stream_index()`. To me it looks like the change is not
> fully complete, so I would propose to revert until fixed.

feel free to revert it or fix it. But please make sure it gets into release/8.0

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".