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

[Justin Ruggles]

Ramiro Polla wrote:
> 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).

If you decide on using channels + request_channels + output_channels,
here is what I think should happen.

demuxer/parser : set channels to the number of source channels,
completely ignoring request_channels.

decoder : during init(), set channels to the number of source channels,
set output_channels to what it will output, trying if it can to honor
request_channels.

The user can see if output_channels matches request_channels after
decoder init.

-Justin