Hi!
Wolfram Gloger wrote:
>>And once it's in sync, it should stay in sync. Advancing to the
>>next item should not have any effect as long as `bufferposition' (the
>>index into the buffer) isn't updated as well.
>
>
> But the bufferposition is updated in mpgfile::savempg() by discarding:
>
> bytes=nx->bufferposition-it->bufferposition
>
> bytes. So, if the list of items ends in an unaligned frame,
> next time audio_addpts() is called we are "out of sync" again.
Hmm... `bytes' is the distance between two items, and should be an
integer multiple of the frame size.
>>By the way, how do you detect the sync failures?
>
>
> By debugging :-) I noticed audio frames were written with size
> 1184 even though 576 should have been the size always.
Is it always 1184, or does the value differ?
>>And how often do they
>>happen?
>
>
> The situation I described happens relatively rarely, about once
> in 700MB perhaps (for an unaligned stream).
>
>
>>Well, if they're unaligned, all of them usually are. The question is how
>>often dvbcut will run out of sync. Probably not with every single frame
>>or (PES) packet. Maybe once with every call to audio_addpts()?
>
>
> Yes I think so.
No, if the error occurs once in a while, it must be something else.
audio_addpts() is called rather frequently. But mpgfile::savempg is
called about every 20...30 minutes (once for every cut segment).
>>Actually, testing for `pos+2>=len' at that point was a bug. It should
>>have been `pos > len' because mpaframe updated pos to point at the end
>>of the frame (or even further). Therefore, it is possible that pos==len
>>and ptsplus!=0 if frames are aligned.
>
>
> Yes that is possible. But, again, I think breaking out of the loop
> here "early" is not a problem (such a bug with skipped frames would
> have been found I think). The buffer will be shifted only by sizes of
> parsed frames, leaving unparsed frames at the end is ok as they will
> be handled by the next call to audio_addpts.
If everything else works okay, yes.
>>But you're right - after re-indenting the code I noticed that
>>{ac3,mpa}frame() will only return 0 when they reach the end of the buffer.
>
>
> Ok.
>
>
>>>True (and I would argue you cannot be certain that what follows really
>>>is a valid frame),
>>
>>I could also argue that you can't claim a frame being invalid just
>>because it's not followed by another frame ;-)
>
>
> I see what you mean, but I stand by my claim that a valid frame is at
> least followed by a valid frame _header_, otherwise we get many false
> hits (one MPA header alone is just too unspecific). Most robust would
> be to keep a history of a few past header points and a score for the
> most probable next position (the current ffmpeg parser does this), but
> that would make mpaframe a lot more complicated.
I think that should be handled inside the caller, i.e. audio_addpts().
Actually, the whole syncing business should be handled there. ac3frame()
and mpaframe() should just return the size of the frame, or 0 if there
is no valid header.
There is, by the way, an indicator for the most likely next position:
`bufferposition'. But there is also another bug: If audio_addpts() finds
a valid frame, it will insert a new item. But it's not an item for the
frame itself because `bufferposition' has already been updated and
points *after* the frame just checked. :-(
>>>but I thought that the buffer shifts through the
>>>file with only the already framed data being discarded, so when
>>>audio_addpts is called _again_ the last frame (which is now the first
>>>frame) will be detected just fine?
>>
>>It should. Actually, it should be in sync as well. But it isn't :-(
>
>
> Ok, IMHO the root of the problem is that potentially unaligned items
> are used for framing the audio, and items with an artificial
> STREAM_ITEM_FLAG_DATA_ALIGNMENT_INDICATOR are mixed in.
We don't care about that flag any longer. You could as well remove it.
> Instead, we
> should abandon using the PES packet headers for framing and create a
> separate type of item (with a new flag STREAM_ITEM_FLAG_FRAME) for the
> parsed frame boundaries. My next patch will do exactly this.
Beware! The PES packet headers contain the PTS timestamps we need, and
they're also required for file positioning.
>>I guess we're currently barking at the wrong tree. The fact that
>>audio_addpts() obviously doesn't keep sync is not the fault auf
>>{ac3,mpa}frame(). It's the fault of audio_addpts(), or the functions
>>calling it, i.e. mpgfile::savempg() and mpgfile::playaudio(). You
>>probably can forget about the latter because it calls audio_addpts()
>>only once.
>
>
> Yes, my next patch will more or less rewrite audio_addpts. But I hit
> the issue with the false hits from mpaframe() and I wanted to fix that
> first.
>
>
>>>I tested with several streams and can't see any change in behaviour.
>>
>>How did you verify that - compare the output files?
>
>
> Yes, I just retested on a different machine. Compiled vanilla
> dvbcut-35, exported DVD from a complicated project with several
> cuts (and mpeg audio of course). Then redid with my patch applied.
> The resulting multi-GB files are binary identical.
Hmm. Doesn't that mean that your patch has no effect at all? Or, vice
versa, that the original code worked fine even when there were sync
problems now and then?
The reason probably is that mpgfile::savempg starts audio PTS processing
2 seconds *before* each start point. By the time it reaches the start
PTS, it should already be in sync.
--
Michael "Tired" Riepe <[EMAIL PROTECTED]>
X-Tired: Each morning I get up I die a little
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
DVBCUT-user mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dvbcut-user
