Re: [DVBCUT-user] [PATCH] Fix audio framing in streamdata.cpp
Hi,
Hmm... `bytes' is the distance between two items, and should be an
integer multiple of the frame size.
But items also include the (sometime) non-aligned PES packet headers;
these are not deleted in audio_addpts. For those, the distance is
arbitrary, see the log output below.
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?
Ok, I have just applied
--- dvbcut-orig/src/mpgfile.cpp~Sun May 27 17:24:25 2007
+++ dvbcut-orig/src/mpgfile.cpp Mon May 28 12:15:51 2007
@@ -619,6 +619,7 @@
if (bytes0)
{
pts_t pts=audiopts[a]-audiooffset[a];
+ fprintf(stderr, mux.put audio %d %lld\n, bytes, pts);
mux.putpacket(audiostream(a),sd-getdata(),bytes,pts,pts,MUXER_FLAG_KEY);
sd-discard(bytes);
and I append the output for otherwise unchanged dvbcut-35
for a stream with unaligned audio.
Maybe you can make something out of it?
It doesn't include the case with false positive from mpaframe, though.
No, if the error occurs once in a while, it must be something else.
I have two separate problems, but the one with false positive from
mpaframe() happens very rarely, and is handled by my patch.
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.
Yes! That is exactly what my patch does.
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. :-(
Yes! That is why I changed the {mpa,ac3}frame calling so this
can be handled correctly (in a followup patch).
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.
Ok..
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'm not deleting them, just not using them for audio output.
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 patch just fixes the very rare false positive from mpaframe. It
doesn't solve the sync problem. I think I'll post my current complete
patch in another thread so maybe everything becomes clearer.
Regards,
Wolfram
mux.put audio 960 3721
mux.put audio 576 5881
mux.put audio 288 8041
mux.put audio 864 10201
mux.put audio 576 12361
mux.put audio 384 14521
mux.put audio 768 16681
mux.put audio 576 18841
mux.put audio 480 21001
mux.put audio 672 23161
mux.put audio 576 25321
mux.put audio 576 27481
mux.put audio 576 29641
mux.put audio 576 31801
mux.put audio 576 33961
mux.put audio 96 36121
mux.put audio 1056 38281
mux.put audio 576 40441
mux.put audio 192 42601
mux.put audio 960 44761
mux.put audio 576 46921
mux.put audio 288 49081
mux.put audio 864 51241
mux.put audio 576 53401
mux.put audio 384 55561
mux.put audio 768 57721
mux.put audio 576 59881
mux.put audio 480 62041
mux.put audio 672 64201
mux.put audio 576 66361
mux.put audio 576 68521
mux.put audio 576 70681
mux.put audio 576 72841
mux.put audio 576 75001
mux.put audio 96 77161
mux.put audio 1056 79321
mux.put audio 576 81481
mux.put audio 192 83641
mux.put audio 960 85801
mux.put audio 576 87961
mux.put audio 288 90121
mux.put audio 864 92281
mux.put audio 576 94441
mux.put audio 384 96601
mux.put audio 768 98761
mux.put audio 576 100921
mux.put audio 480 103081
mux.put audio 672 105241
mux.put audio 576 107401
mux.put audio 576 109561
mux.put audio 576 111721
mux.put audio 576 113881
mux.put audio 576 116041
mux.put audio 96 118201
mux.put audio 1056 120361
mux.put audio 576 122521
mux.put audio 192 124681
mux.put audio 960 126841
mux.put audio 576 129001
mux.put audio 288 131161
mux.put audio 864 133321
mux.put audio 576 135481
mux.put audio 384 137641
mux.put
[DVBCUT-user] [PATCH] Fix audio framing in streamdata.cpp
Hi,
I still get false hits from streamdata.cpp:mpaframe() for audio
streams where MPEG audio frames aren't aligned to packet boundaries.
The reason is that mpaframe() simply looks for a possible MPA header,
and then looks for another MPA header at an offset of the framesize
_or anywhere later_. So the following can happen:
Buffer Offset Content
===
x MPA frame start
...
x+120 Packet boundary (Item points here)
...
x+129 Invalid MPA (is_mpa() returns true but it's
not really the start of a frame)
...
x+576 MPA frame start
...
x+2*576MPA frame start
...
mpaframe() in this case will happily detect an MPA frame between x+129
and x+2*576. The correct frame, however, is between x+576 and
x+2*576. With the current code, this means that the frame starting at
x+576 is missed.
The attached patch fixes this by only considering MPA headers which
are _exactly_ one frame size apart. (All other MPEG parsers that I
know do this also.) It also fixes a potential buffer overflow in
ac3frame().
I changed the interface for mpaframe() and ac3frame() so they return
the start _and_ size of valid audio frames. This will be needed in
another patch which I will submit soon.
BTW the needsync code in audio_addpts() IMHO does absolutely nothing
and should be removed. All items which have no pts are removed at
the start of the function, and only items which do have pts are added.
So, the following loop always terminates immediately:
if (needsync) {
needsync=false;
for(;it!=items.end();++it)
if (it-headerhaspts())
// header carries PTS
break;
...
Regards,
Wolfram.
dvbcut-frame0.diff
Description: Binary data
-
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
DVBCUT-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dvbcut-user
Re: [DVBCUT-user] [PATCH] Fix audio framing in streamdata.cpp
Hi!
Wolfram Gloger wrote:
I still get false hits from streamdata.cpp:mpaframe() for audio
streams where MPEG audio frames aren't aligned to packet boundaries.
The reason is that mpaframe() simply looks for a possible MPA header,
and then looks for another MPA header at an offset of the framesize
_or anywhere later_. So the following can happen:
Buffer Offset Content
===
x MPA frame start
...
x+120 Packet boundary (Item points here)
...
x+129 Invalid MPA (is_mpa() returns true but it's
not really the start of a frame)
...
x+576 MPA frame start
...
x+2*576MPA frame start
...
mpaframe() in this case will happily detect an MPA frame between x+129
and x+2*576. The correct frame, however, is between x+576 and
x+2*576. With the current code, this means that the frame starting at
x+576 is missed.
Yep. That's why there is the sync code. After a while, the program
should lock to the correct position automatically.
The attached patch fixes this by only considering MPA headers which
are _exactly_ one frame size apart. (All other MPEG parsers that I
know do this also.) It also fixes a potential buffer overflow in
ac3frame().
That will work fine in a perfect world where no transmission errors occur.
I noticed that you drop out of the loop in audio_addpts() (line 182 in
your version) when framesize == 0. Doesn't that mean that a single
missing frame header will stop audio pts processing prematurely?
I changed the interface for mpaframe() and ac3frame() so they return
the start _and_ size of valid audio frames. This will be needed in
another patch which I will submit soon.
BTW the needsync code in audio_addpts() IMHO does absolutely nothing
and should be removed. All items which have no pts are removed at
the start of the function, and only items which do have pts are added.
So, the following loop always terminates immediately:
if (needsync) {
needsync=false;
for(;it!=items.end();++it)
if (it-headerhaspts())
// header carries PTS
break;
I agree that the for loop seems to terminate immediately and should be
removed. But what about the rest of the code? It used to be executed
whenever there was a frame synchronization problem. With your patch,
it's only executed once at the beginning of the outer (while) loop
because you deleted the assignment needsync=true later on (line 182 in
revision 35) and decided to leave the loop instead.
Besides that, your code will never return the length of the last frame
that begins in the buffer because {ac3,mpa}frame() can't access the
following frame header. But what happens if the frames are aligned?
I'm afraid you opened a big can of worms here.
--
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
DVBCUT-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dvbcut-user
Re: [DVBCUT-user] [PATCH] Fix audio framing in streamdata.cpp
Delivery-Date: Sun, 27 May 2007 17:00:50 +0200
Received-SPF: pass (mxeu11: domain of lists.sourceforge.net designates
66.35.250.225 as permitted sender) client-ip=66.35.250.225; [EMAIL
PROTECTED]; helo=lists-outbound.sourceforge.net;
Hi,
Yep. That's why there is the sync code. After a while, the program
should lock to the correct position automatically.
Sorry, I don't follow. Whenever an unaligned item is encountered
(through
it=nx;
++nx;
pts=it-headerpts();
) at the end of the loop, the situation I described can
happen again.
I agree it wouldn't be a problem if the situation only occurred once
at the start of the stream, but we have already seem that for some
streams unaligned packets happen regularly.
That will work fine in a perfect world where no transmission errors occur.
I noticed that you drop out of the loop in audio_addpts() (line 182 in
your version) when framesize == 0. Doesn't that mean that a single
missing frame header will stop audio pts processing prematurely?
Indeed, that is a perhaps a mistake -- maybe I should have posted the
complete patch which I have in mind -- but I believe that doesn't
change anything compared to the original version, see below.
I agree that the for loop seems to terminate immediately and should be
removed. But what about the rest of the code? It used to be executed
whenever there was a frame synchronization problem. With your patch,
it's only executed once at the beginning of the outer (while) loop
because you deleted the assignment needsync=true later on (line 182 in
revision 35) and decided to leave the loop instead.
Yes that is perhaps a mistake. But consider the original code.
How can needsync=true come about?
Only if ptsplus==0, and that means that mpaframe() or
ac3frame() has returned 0. And I can see no way of that
happening without pos+2=len, which means that the condition
if (pos+2=len)
break;
in audio_addpts() would already have left the loop!
Besides that, your code will never return the length of the last frame
that begins in the buffer because {ac3,mpa}frame() can't access the
following frame header.
True (and I would argue you cannot be certain that what follows really
is a valid frame), 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?
But what happens if the frames are aligned?
Sorry, I don't understand that question.
I'm afraid you opened a big can of worms here.
I tested with several streams and can't see any change in behaviour.
Can you?
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
DVBCUT-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dvbcut-user
Re: [DVBCUT-user] [PATCH] Fix audio framing in streamdata.cpp
Hi again!
Wolfram Gloger wrote:
Delivery-Date: Sun, 27 May 2007 17:00:50 +0200
Received-SPF: pass (mxeu11: domain of lists.sourceforge.net designates
66.35.250.225 as permitted sender) client-ip=66.35.250.225; [EMAIL
PROTECTED]; helo=lists-outbound.sourceforge.net;
Hi,
Yep. That's why there is the sync code. After a while, the program
should lock to the correct position automatically.
Sorry, I don't follow. Whenever an unaligned item is encountered
(through
it=nx;
++nx;
pts=it-headerpts();
) at the end of the loop, the situation I described can
happen again.
This code really is confusing. And sometimes wrong.
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. 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.
By the way, how do you detect the sync failures? And how often do they
happen?
I agree it wouldn't be a problem if the situation only occurred once
at the start of the stream, but we have already seem that for some
streams unaligned packets happen regularly.
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()?
That will work fine in a perfect world where no transmission errors occur.
I noticed that you drop out of the loop in audio_addpts() (line 182 in
your version) when framesize == 0. Doesn't that mean that a single
missing frame header will stop audio pts processing prematurely?
Indeed, that is a perhaps a mistake -- maybe I should have posted the
complete patch which I have in mind -- but I believe that doesn't
change anything compared to the original version, see below.
I agree that the for loop seems to terminate immediately and should be
removed. But what about the rest of the code? It used to be executed
whenever there was a frame synchronization problem. With your patch,
it's only executed once at the beginning of the outer (while) loop
because you deleted the assignment needsync=true later on (line 182 in
revision 35) and decided to leave the loop instead.
Yes that is perhaps a mistake. But consider the original code.
How can needsync=true come about?
Only if ptsplus==0, and that means that mpaframe() or
ac3frame() has returned 0. And I can see no way of that
happening without pos+2=len, which means that the condition
if (pos+2=len)
break;
in audio_addpts() would already have left the loop!
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.
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.
Besides that, your code will never return the length of the last frame
that begins in the buffer because {ac3,mpa}frame() can't access the
following frame header.
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 ;-)
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 :-(
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.
I tested with several streams and can't see any change in behaviour.
How did you verify that - compare the output files?
--
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
DVBCUT-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dvbcut-user
