| Message ID | 1620949077-2936-1-git-send-email-joshua.allmann@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/flvdec: Ignore the first two data/subtitle streams. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Thu, 13 May 2021 at 16:38, Josh Allmann <joshua.allmann@gmail.com> wrote: > > Previously, one or the other would have been ignored, but not both. > Since the probe terminates at three streams, it could exit > prematurely if both data and subtitles are present along with > slightly trailing media, usually video trailing audio. > > Trailing media is common in RTMP, and encoders write strange metadata. > --- Here's a trac ticket with some more context: https://trac.ffmpeg.org/ticket/9241 And a sample that exhibits the problem: https://trac.ffmpeg.org/attachment/ticket/9241/flv-probe-missing-streams.flv Josh
Hi, On Fri, 14 May 2021, Josh Allmann wrote: > On Thu, 13 May 2021 at 16:38, Josh Allmann <joshua.allmann@gmail.com> wrote: >> >> Previously, one or the other would have been ignored, but not both. >> Since the probe terminates at three streams, it could exit >> prematurely if both data and subtitles are present along with >> slightly trailing media, usually video trailing audio. >> >> Trailing media is common in RTMP, and encoders write strange metadata. >> --- > > Here's a trac ticket with some more context: > https://trac.ffmpeg.org/ticket/9241 > > And a sample that exhibits the problem: > https://trac.ffmpeg.org/attachment/ticket/9241/flv-probe-missing-streams.flv Just FYI and sorry, I'm planning on looking at this patch and have it pinned to look at later, but I haven't yet had time to actually take a look. Hoping to get to it soon... // Martin
On Thu, 13 May 2021, Josh Allmann wrote: > Previously, one or the other would have been ignored, but not both. > Since the probe terminates at three streams, it could exit > prematurely if both data and subtitles are present along with > slightly trailing media, usually video trailing audio. > > Trailing media is common in RTMP, and encoders write strange metadata. > --- > libavformat/flvdec.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > index 4b9f46902b..1be8d98618 100644 > --- a/libavformat/flvdec.c > +++ b/libavformat/flvdec.c > @@ -134,18 +134,32 @@ static void add_keyframes_index(AVFormatContext *s) > } > } > > +static int is_ignorable(enum AVMediaType codec_type) > +{ > + switch(codec_type) { > + case AVMEDIA_TYPE_SUBTITLE: > + case AVMEDIA_TYPE_DATA: > + return 1; > + } > + return 0; > +} > + > static AVStream *create_stream(AVFormatContext *s, int codec_type) > { > + int streams_to_ignore = 0, nb_streams = 0; > FLVContext *flv = s->priv_data; > AVStream *st = avformat_new_stream(s, NULL); > if (!st) > return NULL; > st->codecpar->codec_type = codec_type; > - if (s->nb_streams>=3 ||( s->nb_streams==2 > - && s->streams[0]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE > - && s->streams[1]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE > - && s->streams[0]->codecpar->codec_type != AVMEDIA_TYPE_DATA > - && s->streams[1]->codecpar->codec_type != AVMEDIA_TYPE_DATA)) > + > + if (s->nb_streams >= 1) > + streams_to_ignore += is_ignorable(s->streams[0]->codecpar->codec_type); > + if (s->nb_streams >= 2) > + streams_to_ignore += is_ignorable(s->streams[1]->codecpar->codec_type); > + > + nb_streams = s->nb_streams - streams_to_ignore; > + if (nb_streams >= 2) > s->ctx_flags &= ~AVFMTCTX_NOHEADER; Overall, the idea of the patch seems sensible, but it could be done slightly differently... Instead of explicitly inspecting streams [0] and [1], wouldn't it be more straightforward to just loop over all existing streams and count the ones where is_ignorable returns 0? Otherwise, if you'd have e.g. the streams {audio, data, subtitle}, you'd still end up with nb_streams = 2 which probably isn't what you meant? Separately, it'd be great if you could, while touching this code, write some comments about what's happening here - it's rather non-obvious code (especially with the double negations, in clearing the NOHEADER flag, and what that flag implies). // Martin
diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c index 4b9f46902b..1be8d98618 100644 --- a/libavformat/flvdec.c +++ b/libavformat/flvdec.c @@ -134,18 +134,32 @@ static void add_keyframes_index(AVFormatContext *s) } } +static int is_ignorable(enum AVMediaType codec_type) +{ + switch(codec_type) { + case AVMEDIA_TYPE_SUBTITLE: + case AVMEDIA_TYPE_DATA: + return 1; + } + return 0; +} + static AVStream *create_stream(AVFormatContext *s, int codec_type) { + int streams_to_ignore = 0, nb_streams = 0; FLVContext *flv = s->priv_data; AVStream *st = avformat_new_stream(s, NULL); if (!st) return NULL; st->codecpar->codec_type = codec_type; - if (s->nb_streams>=3 ||( s->nb_streams==2 - && s->streams[0]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE - && s->streams[1]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE - && s->streams[0]->codecpar->codec_type != AVMEDIA_TYPE_DATA - && s->streams[1]->codecpar->codec_type != AVMEDIA_TYPE_DATA)) + + if (s->nb_streams >= 1) + streams_to_ignore += is_ignorable(s->streams[0]->codecpar->codec_type); + if (s->nb_streams >= 2) + streams_to_ignore += is_ignorable(s->streams[1]->codecpar->codec_type); + + nb_streams = s->nb_streams - streams_to_ignore; + if (nb_streams >= 2) s->ctx_flags &= ~AVFMTCTX_NOHEADER; if (codec_type == AVMEDIA_TYPE_AUDIO) { st->codecpar->bit_rate = flv->audio_bit_rate;