[MPlayer-dev-eng] [PATCH] AVParser for audio support

[Uoti Urpala]

On Thu, 2009-08-27 at 00:56 +0200, Reimar Döffinger wrote:
> On Thu, Aug 27, 2009 at 12:19:44AM +0300, Uoti Urpala wrote:
> > On Wed, 2009-08-26 at 22:09 +0200, Reimar Döffinger wrote:
> > > On Wed, Aug 26, 2009 at 10:52:48PM +0300, Uoti Urpala wrote:
> > > > How about doing the
> > > > parsing in ad_ffmpeg? Is there a reason to expect a practical benefit
> > > > from doing it elsewhere?
> > > 
> > > Well, what is the benefit of doing it in ad_ffmpeg? My incomplete attempt
> > > looked at least as bad and won't even work with mencoder an -oac copy.
> > 
> > At least it should avoid the "if (ds == ds->demuxer->audio)" testing and
> > creation of the parser in the middle of a ds_add_packet() call,
> 
> Where else to create it? There's no perfect solution I can find, when
> creating the stream or somewhen after wastes resources with many
> audio/video streams that might never play.

>From resource management point of view having the parser exist whenever
the decoder does seems natural.

> When switching audio as far as I can tell would need a special case
> for the default stream.
> Neither do I see a good solution to avoid the "if (ds ==
> ds->demuxer->audio)" though I don't know what the particular thing you
> want to avoid is, should it work with video, too, or some other thing?

If you do the parsing in ad_ffmpeg then there should be no need for such
tests (as obviously it's handling audio only).

IMO the generic packet functions should avoid dependency on packet type
details when possible; and that particular way of writing the test makes
supporting multiple audio streams harder.


> > allow sane(r) handling of any possible error cases. The
> > av_parser_parse2() documentation also mentions some flush-at-EOF
> > functionality and that cannot really be supported at the packet add
> > side.
> 
> You could probably hack a call to packet_add before concluding it is
> really EOF. Doing everything while reading packets means you need an
> extra buffer either for the extra generate packets or for the
> incompletely processed packet (that is the same as for doing it in
> ad_ffmpeg).

With some reasonably simple changes to the demuxer.c API you could
continue processing the same incompletely processed packet over multiple
calls without allocating a separate buffer. I think a way to tell how
much of the packet you used and a function to get a possibly partial
packet with information about how much of it was used earlier should be
enough; demuxer.c already keeps such information for the
demux_read_data() interface.

If you do parsing inside a demuxer.c read-side function then I don't see
how that would need anything extra for buffering, as long as you only
support reading the parsed data as whole packets (so there's no need to
implement an individual read position variable for them).


> > > > Even if it's needed for multiple uses either
> > > > adding a separate parsing function or doing parsing in demuxer.c
> > > > functions when taking packets _out of_ the queue (so the information
> > > > from the demuxer is kept intact in the queue) could be preferable.
> > > 
> > > On the other hand the information before parsing is unlikely to be really
> > > too correct (particularly pts/dts). In case of AC3 in AVI there is also the
> > > annoyance that packets seem to vary greatly in size (and thus duration)
> > > before parsing.
> > 
> > Why is that an annoyance? BTW in that case it's probably better to only
> > set the pts for the first parsed packed corresponding to a container
> > packet and leave it unset for the rest (instead of creating multiple
> > packets with the same pts like your current patch does).
> 
> No, the correct solution is to use the correct pts/dts (don't actually
> know which) from the parser I just hadn't yet figured out how the API
> for that works.

That's unlikely to be any better for playback in practice, unless the
parser can somehow give a more accurate timestamp for the start of the
first packet (I'm not sure how it could without assuming a specific way
how timestamps were assigned to combined packets in the file).

> Same is probably true for pos, though that might depend on what exactly
> pos is supposed to be.
> And it is an "annoyance" because the varying duration in some parts of
> MPlayer at least in the past sometimes cause "timestamp jumps". It may
> have been in something so unimportant as the status line.

I don't think any problem quite like that has existed. Large packets
could have made inaccuracies in bitrate-based time estimation more
visible; but even in that case the main problem was the inaccuracy
itself, and the effect would have depended on absolute size rather than
variation.


> > Doing the parsing in ad_ffmpeg would avoid the need to attempt
> > implementing a general-case one-to-many packet duplication function.
> 
> Well, AVParser is supposed to be that one-to-many thing already, and
> deficiencies in that regard should probably be fixed in FFmpeg.
> Some issues I saw with it actually don't exist, though it has 2
> disadvantages:
> 1) it does not work with video or subtitles (mine only doesn't because
> it has to differentiate between sh_audio_t/sh_video_t)

That depends on how likely it is that parsing will be needed for those.
If there are some likely uses then the basic parser logic should
probably be in a shared function (though that could still be called from
ad_ffmpeg and other decoders).

> 2) it does not work with mencoder

As mentioned earlier I'm not sure whether modifying the track with
unconditional parsing when told to copy it is actually an improvement or
not. And anyway I don't want to base any design decisions on MEncoder
features when it's so clearly rotting overall (better let it rot faster
alone than let it drag MPlayer with it).

> 3) you have to buffer the last packet in case it was so large you
> can't decode all of it

With the above demuxer.c changes this wouldn't require any separate
buffering implementation.