Hi,
> This code really is confusing. And sometimes wrong.
Agreed..
> I don't understand how dvbcut can actually encounter so many unaligned
> frames at all. audio_addpts() operates on a single buffer that
> (theoretically, i.e. if the file isn't damaged) contains a number of
> consecutive frames, plus maybe a partial frame at the beginning and at
> the end.
Yes.
> 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.
> 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.
> 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.
> 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.
> 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.
> > 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. 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.
> 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.
Regards,
Wolfram
-------------------------------------------------------------------------
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
