[FFmpeg-devel] TrueHD track in EVO not playable/testable with ffplay

[Ramiro Polla]

On Fri, Jul 10, 2009 at 5:26 PM, Baptiste
Coudurier<baptiste.coudurier at gmail.com> wrote:
> On 07/10/2009 11:47 AM, Ramiro Polla wrote:
>> On Thu, Jul 9, 2009 at 10:42 PM, Baptiste
>> Coudurier<baptiste.coudurier at gmail.com> ?wrote:
>>> On 07/09/2009 05:38 PM, Ramiro Polla wrote:
>>>> 2009/7/9 M?ns Rullg?rd<mans at mansr.com>:
>>>>> Michael Niedermayer<michaelni at gmx.at> ? ?writes:
>>>>>> On Wed, Jul 08, 2009 at 11:01:57PM -0700, Baptiste Coudurier
>>>>>> wrote:
>>>>>>> On 07/08/2009 07:47 PM, Michael Niedermayer wrote:
>>>>>>>> my tired mind says we need a coded_channels representing
>>>>>>>> the number of channels stored in the stream and a
>>>>>>>> seperate field that represents the decoder output
>>>>>>>> channels
>>>>>>>
>>>>>>> I suggest to keep ->channels as the coded channels and to
>>>>>>> use another field like output_channels.
>>>>
>>>> Wouldn't this break API? Programs currently check avctx->channels
>>>> to know how much data there is. If we make them check
>>>> avctx->output_channels we'd need to bump major (and break
>>>> something else so people know they need to update and we hope
>>>> they stumble upon a place that explains this change too).
>>>
>>> It breaks the API anyway since libavformat sets ->channels and in
>>> this case must not set it. libavformat is libavcodec user too :)
>>>
>>> IMHO ->channels is more obvious to people and avoids changing
>>> demuxer and muxer code.
>>>
>>> Also there was no guarantee that ->channels was set to
>>> request_channels, MLP is a proof.
>>>
>>> ->output_channels is clear and obvious IMHO.
>>>
>>>>>> I wouldnt mind, if we wouldnt have coded_width&
>>>>>> coded_height in there
>>>>>
>>>>> This is different. ?Unless I am mistaken, coded_width and
>>>>> coded_height indicate the encoded width/height of a video
>>>>> including any padding (e.g. mod-16) required by the codec but
>>>>> never meant to be displayed.
>>>>>
>>>>> In the case at hand, we have a number of audio channels, all of
>>>>> which are meaningful, but which may be downmixed before
>>>>> output.
>>>>
>>>> By looking at the doxy in avcodec.h I don't think coded_ is the
>>>> best too.
>>>>
>>>> I attached a patch to see if this is what you mean by adding a
>>>> new field (I named it coded_channels like michael suggested but I
>>>> don't really like the name as I pointed out).
>>>>
>>>> Now would this mean that all decoders should be changed to
>>>> "avctx->coded_channels = avctx->channels =<number of channels>"?
>>>> Or only the decoders that support downmixing
>>>
>>> Decoders supporting downmixing should set output_channels to what
>>> is output.
>>
>> Let me see if I understood what you mean. In this case,
>> avctx->channels should be set to 6 in the parser, and
>> output_channels set to the downmixed numbed of channels in the
>> decoder only if(request_channels),
>
> Yes.
>
>> otherwise it's left as 0. av_find_stream_info() should read frames
>> while(!channels || (request_channels&& !output_channels)).
>
> I think av_find_stream_info must stop as soon as 'channels' is found.
>
> 'request_channels' and 'output_channels' is a decoding feature, not info
> nor parsing.

Where can we guarantee to the user that his request_channels has been
validated? In ffplay.c we have:
av_find_stream_info()
set request_channels
avcodec_find_decoder()
avcodec_open()
--- channels is considered to be final.

So after avcodec_open() it should have taken request_channels into
account, but avcodec_open() doesn't decode frames, and in mlp's case
av_find_stream_info() didn't find the proper number of channels.

Currently the files that use request_channels are:
aac_ac3_parser.c - sets channels on the parser
ac3dec.c - sets channels on _init()
dca.c - sets channels on first decode_frame()
libfaad.c - sets channels on _init()

In dca.c, channels is not set from av_find_stream_info(), so frames
are decoded and channels is set on the first frame. In mlp's case
channels is set to some value from the parser that doesn't take
request_channels into account.

How about adding av_request_channels() so the user can be sure
request_channels has been validated? (and if he calls that function,
he should always use avctx->output_channels).

>> Then users are expected to always use output_channels if they set
>> request_channels, and channels otherwise. Or we could make
>> av_find_stream_info() always set output_channels at the end, and we
>> expect users to always use output_channels from now on.
>
> I don't av_find_stream_info should mess with ->output_channels since it is a
> decoding feature.
>
> I don't mind your idea, that is if user sets ->request_channels then
> ->output_channels must be set to what is output. If ->request_channels is
> not set then ->output_channels is not set and is implicitly ->channels if I
> understand correctly.
>
> Or decoder must always set output_channels.