xiph / vorbis-tools

Command-line tools for creating and playing Ogg Vorbis files.
GNU General Public License v2.0
73 stars 28 forks source link

minor issue: granulepos of the last packet of the cut segment (vcut.c) #25

Open Alpt opened 4 years ago

Alpt commented 4 years ago

I believe the following patch should be applied in process_audio_packet:

                /* Set it! This 'truncates' the final packet, as needed. */
-               packet->granulepos = rel_cutpoint;
+               packet->granulepos = rel_cutpoint - vs->samples_cut;
                cut_on_eos = packet->e_o_s;

line 426 of vcut.c, commit 70d0d8e84d272047e8328d376bb983e897e3c670

petterreinholdtsen commented 4 years ago

[Alpt]

I believe the following patch should be applied in process_audio_packet:

Can you tell us why you believe so, and is there a test case to detect the problem it solves?

-- Vennlig hilsen Petter Reinholdtsen

Alpt commented 4 years ago

In line 456, packet->granule_pos is calculated as follow:

        packet->granulepos = vs->granulepos
                - vs->initial_granpos - vs->samples_cut;

Instead, in line 426, it is rel_cutpoint, which is calculated as

vs->vi.rate * (s->next_segment->cuttime - s->prevstream_time)

(this if segment->cuttime >= 0) where s->prevstream_samples += vs->granulepos - vs->initial_granpos

What I am trying to say is that both in 426 and 456 (granulepos - initial_granpos) is considered, but in 456 also vs->samples_cut is considered.

The problem to be observed should be that the last granulepos of the cut segment is wrong if vs->samples_cut is greater than 0.

I am working on a fork of vcut and I was observing this. I solved it with the mentioned patch. As soon as I have time, I'll try to provide a proof of concept.

Best regards.

Alpt commented 4 years ago

Ok, I don't know if it happens in other cases, e.g. chained streams, but it surely happens if you perform more than one cut. To perform more than one cut, set segment->next to another valid vcut_segment. See https://github.com/xiph/vorbis-tools/compare/master...Alpt:pof-ok

Here is the output of running the PoC


# the input file is 10 seconds long

$ soxi ~/Documents/tone-440.ogg  | grep -i duration
Duration       : 00:00:10.00 = 441000 samples = 750 CDDA sectors

# performing a cut each second

$ ./vcut/vcut ~/Documents/tone-440.ogg /tmp/tone-A /tmp/tone-B +1
Processing: Cutting at 1,000000 seconds
/tmp/tone-B-0000
/tmp/tone-B-0001
/tmp/tone-B-0002
/tmp/tone-B-0003
/tmp/tone-B-0004
/tmp/tone-B-0005
/tmp/tone-B-0006
/tmp/tone-B-0007
/tmp/tone-B-0008
/tmp/tone-B-0009
WARNING: found EOS before cutpoint

# each segment has a wrong declared duration. However, if you play it or open with audacity,
   you see 1 second of audio

andrea@convertibile:~/input/software/vorbis-tools-orig$ soxi /tmp/tone-* | grep -i Duration
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:02.00 = 88200 samples = 150 CDDA sectors
Duration       : 00:00:03.00 = 132300 samples = 225 CDDA sectors
Duration       : 00:00:04.00 = 176400 samples = 300 CDDA sectors
Duration       : 00:00:05.00 = 220500 samples = 375 CDDA sectors
Duration       : 00:00:06.00 = 264600 samples = 450 CDDA sectors
Duration       : 00:00:07.00 = 308700 samples = 525 CDDA sectors
Duration       : 00:00:08.00 = 352800 samples = 600 CDDA sectors
Duration       : 00:00:09.00 = 396900 samples = 675 CDDA sectors
Duration       : 00:00:10.00 = 441000 samples = 750 CDDA sectors
Total Duration of 10 files: 00:00:55.00

# indeed, concatenating the segments we get 10 seconds of audio
andrea@convertibile:~/input/software/vorbis-tools-orig$ sox /tmp/tone-* /tmp/concatenated.ogg
andrea@convertibile:~/input/software/vorbis-tools-orig$ soxi /tmp/concatenated.ogg | grep -i Duration
Duration       : 00:00:10.06 = 443664 samples = 754.531 CDDA sectors

Moreover, this issue creates gaps when concatenating the segments. By applying, the patch mentioned above in this issue, all these problems are solved.

$ soxi /tmp/tone-* | grep -i Duration
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Duration       : 00:00:01.00 = 44100 samples = 75 CDDA sectors
Total Duration of 10 files: 00:00:10.00

$ sox /tmp/tone-* /tmp/concatenated.ogg

$ soxi /tmp/concatenated.ogg  | grep -i duration
Duration       : 00:00:10.00 = 441000 samples = 750 CDDA sectors