[MPlayer-dev-eng] [BUG] [PATCH] Ogg/Theora frametime broken on 0-length packets

[David Kuehling]

>>>>> "Reimar" == Reimar Döffinger <Reimar.Doeffinger at gmx.de> writes:

>> > Please provide a better bug report so we can fix this, -demuxer ogg
>> is > unlikely to work forever.
>> 
>> Sorry I don't understand that sentence.  You mean demuxer ogg is
>> deprecated?

> Yes.

>> WRT 'better bug report': what else (apart from a test-case video) do
>> you need?

> For example an explanation why you call it a "leak" (did you use
> e.g. valgrind?), if/how sure you are it is the demuxer and not the
> decoder (as said I think a switch of demuxers implies a switch of
> decoders).

Sorry, I need coffee.  Thought 'better bug report' referred to the
frametime bug I reported.  

Not sure it is a leak at all (forgot to type a question-mark,
i.e. 'leak?'), maybe it needs increasing amounts of memory by design?

Looked quite tough to research that problem, so I ignored it for now.

>> >> Looking closer, I found that ds_get_next_pts() returns the
>> *current* >> pts when the packet read last has zero length.  This is
>> due to the >> check for 'ds->buffer_pos' to see whether data have
>> already been read >> from the *current* packet.  Of course,
>> ds->buffer_pos will stay when >> a zero-length packet was read, so
>> that fails.
>> 
>> > There are multiple possible solutions, the most correct _probably_
>> to > stop the Ogg demuxer from outputting 0-size packets.
>> 
>> This would only work if we would then increase the frametime of the
>> previous video frame accordingly.  I.e. before we enqueue a new video
>> packet we'd first have to go on reading any consecutive 0-size
>> packets (which are AFAIK 5 byte when counting the headers), to
>> determine frametime.  Sure that adding this kind of complexity to the
>> ogg demuxer is the right solution?

> Just not calling ds_add_packet for the 0-size frames should actually
> take care of all that, so it should be about the same amount of code/
> complexity as your patch.  However it might break things for codecs
> where a 0-size packet has a special meaning.

Now that you write that, yes, didn't think about that at all.  Did I say
I need coffee? :) (though it's not really intuitive doing such a thing
on constant-framerate video).  This also gives some sense to the
switch() block in video.c:video_read_frame().

cheers,

David
-- 
GnuPG public key: http://user.cs.tu-berlin.de/~dvdkhlng/dk.gpg
Fingerprint: B17A DC95 D293 657B 4205  D016 7DEF 5323 C174 7D40
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110430/7c5670a4/attachment.asc>