[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:

> On Sat, Apr 30, 2011 at 03:26:12PM +0200, David Kuehling wrote:
>> >>>>> "Reimar" == Reimar Döffinger <Reimar.Doeffinger at gmx.de> writes:
>> > On Sat, Apr 30, 2011 at 03:06:38PM +0200, David Kuehling wrote:
>> >> Hmm, well, AFAIK, for the Theora embedding in Ogg as defined by
>> the >> Theora spec, frametime is a constant.  Variable framerate is
>> only >> possible by using a short frametime, and injecting emtpy
>> packets >> every now and then to pad between actual frames.
>> >> 
>> >> So that's what I was referring to.  Did I overlook something?
>> 
>> > At the very least that theora might be stored in other containers
>> like > mkv without such arbitrary restrictions.
>> 
>> This is why I wrote _Ogg_/Theora is constant-framerate.  You're right
>> my patch checks for theora but should also check for the ogg
>> container.

> I was wondering if it was intentional since the check for Ogg is after
> all right below.

The check for Ogg below is more a check for the OGM which is a
super-superset of Ogg/Theora (since it allows arbitrary embedding of
video data).  The comment says "override frame_time for variable/unknown
FPS formats:".  Yeah, For OGM it's unknown, for Ogg/Theora it's known,
but can't handle that just in the switch.

>> > Then there's the thing that I am unsure that e.g. the FFmpeg muxer
>> > does behave like this.
>> 
>> You really mean muxer not demuxer?

> Yes.

From what I hear over at the theora mailinglists FFmpeg's Ogg muxer is
pretty sub-optimal when compared to ffmpeg2theora's muxer, maybe not a
good to idea to use it as a reference :)

>> > Not sure how "short frametime" is defined, but I would take a guess
>> > that this might be broken by your change.
>> 
>> "Short frametime" is not defined.  Using a short frametime is just a
>> hack to emulate variable-framerate video with a fixed-framerate video
>> format such as Ogg/Theora.  No without my change that wouldn't work
>> at all, because all padding frames that come as 0-sized packets would
>> get a frametime of 0 assigned, thus the video plays back at
>> 1/short_frametime instead of the actual variable framerate of the
>> video.

> I must be something missing in all this, because the frame time
> calculation you disable is based on pts, so in theory the delays
> should at most shift to the wrong place.  

The PTS value is computed from the frametime in the demux_ogg.c.
Ogg/Theora has a linear (integer) frame-number encoded in the granulpos.
So pts := frametime*frameno.  Then computing (pts1-pts0) is just like
comuting (frametime*(frameno+1) - frametime*frameno).  This yields
'frametime', so is a no-op which is why I disable it.  (but it would do
no harm, if ds_get_next_pts() worked).

> Which is where my other theory comes from, that what really does wrong
> is that d_video->pts is updated incorrectly.  Another possibility
> would be to change the buffer_pos condition to also use the next
> packet pts when the buffer size is 0.  Currently I am unsure which
> solution makes most sense, but yours unfortunately right now is the
> one I see most potential issues with (also think of other video
> formats like VP8 in Ogg etc.).

Ogg is not generic in that you can not embed just any video content in
it.  I don't know of a specification for an embedding of VP8 in Ogg.
(This is one of the reasons BTW why demux_ogg.c has to be so ugly.)

Instead of using buffer_pos can't one count the number of enqueued
vs. dequeued packets and use those instead of a byte offset?

ds_get_next_pts() with all the interactions was too complex for me to
attempt to fix, that's why the simpler ogg/theora specific fix.

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/7eaea8fd/attachment.asc>