[FFmpeg-devel] avformat/mp3dec: set default disposition

Submitted by Gyan on Aug. 24, 2019, 9:28 a.m.

Details

Message ID 423ea570-a0a3-ece0-c2e7-d5ecf7d3e0b1@gyani.pro
State New
Headers show

Commit Message

Gyan Aug. 24, 2019, 9:28 a.m.
Fixes #8091, which reports what I consider a change rather than a 
regression since 11d3b03fcb2baae4324aac9481b9bd4a171d4345 
<http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=11d3b03fcb2baae4324aac9481b9bd4a171d4345>

It may be good practice to set default disposition in all demuxers which 
only accept one stream of a type.

Gyan
From 647968016d28114bd9880daf5b5b728d005e8a69 Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Sat, 24 Aug 2019 14:52:31 +0530
Subject: [PATCH] avformat/mp3dec: set default disposition

Fixes #8091
---
 libavformat/mp3dec.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Hendrik Leppkes Aug. 24, 2019, 11:13 a.m.
On Sat, Aug 24, 2019 at 11:28 AM Gyan <ffmpeg@gyani.pro> wrote:
>
> Fixes #8091, which reports what I consider a change rather than a
> regression since 11d3b03fcb2baae4324aac9481b9bd4a171d4345
> <http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=11d3b03fcb2baae4324aac9481b9bd4a171d4345>
>
> It may be good practice to set default disposition in all demuxers which
> only accept one stream of a type.
>

Any disposition should only be set if the container has any metadata
to indicate them, not blindly be set for no particular reason.
For the ticket in question, if you want to replace a stream in a file,
you should manually ensure its mapped properly.

- Hendrik
Gyan Aug. 24, 2019, 1:34 p.m.
On 24-08-2019 04:43 PM, Hendrik Leppkes wrote:
> On Sat, Aug 24, 2019 at 11:28 AM Gyan <ffmpeg@gyani.pro> wrote:
>> Fixes #8091, which reports what I consider a change rather than a
>> regression since 11d3b03fcb2baae4324aac9481b9bd4a171d4345
>> <http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=11d3b03fcb2baae4324aac9481b9bd4a171d4345>
>>
>> It may be good practice to set default disposition in all demuxers which
>> only accept one stream of a type.
>>
> Any disposition should only be set if the container has any metadata
> to indicate them, not blindly be set for no particular reason.
> For the ticket in question, if you want to replace a stream in a file,
> you should manually ensure its mapped properly.

The ticket reports a change in auto-mapping behaviour. Manual mapping is 
a workaround which I already provided at trac but it doesn't correct the 
regression which should be addressed since I've confirmed that 
auto-mapping is affected for instances beyond that in the ticket e.g.   
ffmpeg -i avi -i mp4 outfile where both files have streams with 
identical parameters.

Maybe a better change is to revert or refine 11d3b03fcb2 since 
disposition was originally introduced to convey and store flags in two 
particular formats: nut and matroska but the ffmpeg tool now 
unconditionally gives *default disposition* weight in selecting streams. 
Note: the MOV demuxer abuses the field by setting default disposition 
based on whether a stream is enabled so you can have a MP4 with N 
streams of a type, all being marked as default.

For containers which only support one stream of a type (A, V or S) or 
raw bitstreams,  metadata can't be expected since there's no choice or 
preference to be signaled; it is implicit that the one and only possible 
track is the 'default' one, so it is not a blind or wayward assignment. 
But this patch was a narrow fix for 8091 so I don't care to press for it.

Gyan
Nicolas George Aug. 24, 2019, 1:37 p.m.
Gyan (12019-08-24):
> The ticket reports a change in auto-mapping behaviour. Manual mapping is a
> workaround which I already provided at trac but it doesn't correct the
> regression which should be addressed since I've confirmed that auto-mapping
> is affected for instances beyond that in the ticket e.g.   ffmpeg -i avi -i
> mp4 outfile where both files have streams with identical parameters.

Is it really a regression? I mean: was the old behaviour really better
in many cases?

Regards,
Gyan Aug. 24, 2019, 1:49 p.m.
On 24-08-2019 07:07 PM, Nicolas George wrote:
> Gyan (12019-08-24):
>> The ticket reports a change in auto-mapping behaviour. Manual mapping is a
>> workaround which I already provided at trac but it doesn't correct the
>> regression which should be addressed since I've confirmed that auto-mapping
>> is affected for instances beyond that in the ticket e.g.   ffmpeg -i avi -i
>> mp4 outfile where both files have streams with identical parameters.
> Is it really a regression? I mean: was the old behaviour really better
> in many cases?
I see default disposition set by only three demuxers among the many 
dozens lavf has. Given the weight to it now, lowest index is no longer 
respected for otherwise near-identical streams. For audio it looks to be 
completely broken, e.g. a stereo stream can be chosen over an 8-channel 
stream.

Gyan

Patch hide | download patch | download mbox

diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 258f19174b..2199e42cd0 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -364,6 +364,7 @@  static int mp3_read_header(AVFormatContext *s)
     st->codecpar->codec_id = AV_CODEC_ID_MP3;
     st->need_parsing = AVSTREAM_PARSE_FULL_RAW;
     st->start_time = 0;
+    st->disposition |= AV_DISPOSITION_DEFAULT;
 
     // lcm of all mp3 sample rates
     avpriv_set_pts_info(st, 64, 1, 14112000);