BrettSheleski / comchap

Commercial detection script to add chapters into video file
MIT License
138 stars 26 forks source link

Missing Last Portion of File #15

Closed arrmo closed 7 years ago

arrmo commented 7 years ago

FYI, the last portion of the file is not included ... from the end of the last line in the EDL file, to the end of the video.

Thanks!

BrettSheleski commented 7 years ago

Please confirm that this is the case. I would think for sure that, or others, would have definitely noticed this before.

arrmo commented 7 years ago

Hi,

I'm 95% sure ... :-). Checked the EDL file, looking at commercial times, and the time of the file before and after. Also checking the video, there is definitely a section missing at the end.

And looking at the code logic - I don't see where a segment is generated (and then joined), from the last entry in the EDL file, to the end of the input .ts .. or am I missing it?

Thanks!

BrettSheleski commented 7 years ago

To clarify, are we talking about comchap or comcut? I'm assuming we're talking about comcut.

BrettSheleski commented 7 years ago

I reviewed comcut, sure enough, it's not handling the last portion of non-commercial content. It is also still using the bc command for math stuff.

I'll have to fix this.

arrmo commented 7 years ago

Yep, comcut. Sorry for the delayed reply, was off at the gym ... ;-). I can dig into this as well if you want, just let me know.

Thanks!

BrettSheleski commented 7 years ago

I just pushed a commit (see 6fd39f9). See if that resolves your issue.

BrettSheleski commented 7 years ago

I see there was an issue in determining the duration of the final chapter. I fixed that and re-pushed (see e86062b). Try again now.

arrmo commented 7 years ago

Hi,

OK, tried to convert a file - but I don't think it's quite right yet. I could definitely be wrong, but here's why I say this ...

Original File = 3:48:57 (13737 seconds) EDL cut = 3480 seconds (see EDL below) Output File (expected) = 10257 seconds Output File (actual) = 2:46:26 (9986 seconds) => Missing ~ 271 seconds, agreed?

Here is the EDL output, 0.00 496.96 0 1008.27 1276.84 0 2160.93 2293.02 0 2740.37 2876.91 0 3044.41 3170.97 0 3699.73 3832.06 0 4069.30 4200.90 0 4487.02 4618.85 0 4783.81 4890.95 0 5147.38 5177.47 0 5207.74 5279.27 0 5509.77 5643.50 0 5825.35 5957.12 0 6230.72 6362.56 0 6688.65 6816.48 0 7232.19 7343.54 0 7785.74 7918.01 0 8828.42 8960.55 0 9410.80 9543.07 0 9953.68 10085.71 0 11006.83 11108.20 0 11612.10 11733.19 0 12025.78 12147.64 0 12800.25 12939.26 0 13403.82 13470.02 0

Thoughts?

Thanks!

arrmo commented 7 years ago

Actually, and I just noticed ... the time from the last commercial (at 13470.02 seconds), to the end of the file (13737 seconds) is 267 seconds => almost exactly what is missing, right?

Trying to check for info, but $metafile doesn't seem to exist (.ffmeta) ... any thoughts?

BrettSheleski commented 7 years ago

By default comcut deletes the ffmeta file. Did you specify --keep-meta?

arrmo commented 7 years ago

Doh! That was me, sorry ... ;-).

OK, here is what I'm seeing - I may be misunderstanding, please correct me if I have it wrong. But here are the first few lines from the EDL, then the ffmeta file, with comments inserted (as '=>arrmo:'),

EDL, 0.00 496.96 0 1008.27 1276.84 0 2160.93 2293.02 0

ffmeta, ;FFMETADATA1 [CHAPTER] TIMEBASE=1/1000 START=496960 END=1008270 =>arrmo: OK, this chapter makes sense, matches EDL [CHAPTER] TIMEBASE=1/1000 START=1008270 =>arrmo: This isn't starting the right place, is it? It should start after the commercial, not the same as above .. or is this after cutting? If it is, perhaps log what is used for cutting also? END=1892360

Thoughts?

Thanks!

BrettSheleski commented 7 years ago

The start and end times in the meta file need to subtract out the amount of time that was removed from where the commercials were. This is why the start and end times do not match between the ffmeta and the edl file. However the difference between each start and end times should still match for each entry.

BrettSheleski commented 7 years ago

To add to this, the start time of one chapter should be the same as the end time of the previous chapter.

arrmo commented 7 years ago

Gotcha - that makes complete sense, was wondering if that was it. But then, here is the last chapter in the file,

[CHAPTER] TIMEBASE=1/1000 START=10022430 END=10486990

Hmmm ... this doesn't match my numbers above, either the expected or actual file length. Let me try to log some more output, try to figure it out.

Thanks!

BrettSheleski commented 7 years ago

Have you tried actually playing the file in question? It may just be an issue with the ffmeta file, which really only affects if you skip forward/back using chapters during playback.

Try skipping to the last chapter, rewind a few seconds, the play till the end. See if the final commercial break was indeed removed (and at the correct time) and if it plays back till the end.

arrmo commented 7 years ago

Yep, just checked the video files. The output ends right before the last commercial - missing the portion after it. Will try to re-run with some console output to see if the last bit of code is being run.

BrettSheleski commented 7 years ago

There's a simple loop at the end that deletes all temporary files used, one for each chapter. You could try commenting-out the rm command inside. Then take a look at the files and see what's up.

arrmo commented 7 years ago

Actually, added an echo as below,

dont forget to add the final part from last commercial to end of file

end=$ffmpegPath -hide_banner -loglevel error -nostdin -i "$infile" 2>&1 | grep Duration | awk '{print $2}' | tr -d , | awk -F: '{ printf "%f", ($1*3600)+($2*60)+$3 }' echo "End: $end"

And I get, End:

Also added in, if [ echo "$end" | awk '{printf "%i", $0 * 1000}' -gt echo "$start" | awk '{printf "%i", $0 * 1000}' ]; then echo "Adding last portion ..."

This echo never gets hit. So it seems the line after "#dont forget to add the final part from last commercial to end of file" is where the issue is, agreed?

arrmo commented 7 years ago

Trying the first portion of that command, I get the response below ... $ffmpegPath -hide_banner -loglevel error -nostdin -i "$infile" 2>&1

[mpeg2video @ 0x3932960] Invalid frame dimensions 0x0. Last message repeated 50 times At least one output file must be specified

So no Duration in the output.

BrettSheleski commented 7 years ago

Ah damn. It looks like finding the total duration of the input file is the issue. I copied that from what I did in comchap. Then I went ahead and changed things to deal with the ffmeta file needing entries to be in milliseconds, while what's in the edl file and what needs to get passed to ffmpeg to make the chapter file needs to be done in seconds.

I'm not going to get to this tonight. My ass is firmly on the couch with my feet up. Pull requests are always welcome.

arrmo commented 7 years ago

But, with ffprobe (-i filename), [mpeg2video @ 0x348c1c0] Invalid frame dimensions 0x0. Last message repeated 50 times [mpegts @ 0x3487580] PES packet size mismatch Last message repeated 1 times Input #0, mpegts, from 'PGA Tour Golf (2003) - 2017-02-19 00 00 00 - Genesis Open Final Round.ts': Duration: 03:48:58.41, start: 12699.904589, bitrate: 14807 kb/s Program 3111 Stream #0:0[0x457]: Video: mpeg2video (Main) ([2][0][0][0] / 0x0002), yuv420p(tv, bt709, top first), 1920x1080 [SAR 1:1 DAR 16:9], Closed Captions, 29.97 fps, 29.97 tbr, 90k tbn, 59.94 tbc Stream #0:10x458: Audio: ac3 ([129][0][0][0] / 0x0081), 48000 Hz, 5.1(side), fltp, 384 kb/s Stream #0:20x459: Audio: ac3 ([129][0][0][0] / 0x0081), 48000 Hz, mono, fltp, 192 kb/s

Then, with the rest of the line. ffmpeg likely works also, but ffprobe returns what is needed.

arrmo commented 7 years ago

LOL - NP, turning in now also (early morning tomorrow), but will continue tomorrow night. Have a nice evening!

BrettSheleski commented 7 years ago

I think it may be the -log_level error stuff I added to quiet down ffmpeg.i didn't think that would cause an issue. Try removing it and rerunning.

BrettSheleski commented 7 years ago

I originally used ffprobe, but opted to go for using just ffmpeg to be more compatible for when specifying ffmpeg-path via command line. I didn't also need to make a ffprobe-path option too. The fewer dependencies the better.

BrettSheleski commented 7 years ago

I'm pretty confident removing the -loglevel error part will fix the issue.

arrmo commented 7 years ago

Tried that, no joy => the issue seems to be the noted error, "At least one output file must be specified"

I can get ffprobe to work (command line below), but I think you want to stick with ffmpeg? If so, we need to find a way around the output file error.

ffprobe -v quiet -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 FILENAME

A link that may help also, https://trac.ffmpeg.org/wiki/FFprobeTips (see Duration)

BrettSheleski commented 7 years ago

Removed -loglevel error and commited. In my simple testing it got the duration just fine. Re-pull and try again.

BrettSheleski commented 7 years ago

nevermind, still issues. Working them out now.

BrettSheleski commented 7 years ago

I think I got it now. I didn't increment the chapter count when writing to the final chapter file. This resulted in the second-to-last chapter file getting overwritten with the final chapter then erroring out later on when cleaning up the chapter files.

arrmo commented 7 years ago

Yep, it seems to work now - thanks! BTW, just a bit odd ... but the first segment is now numbered -2. Does that sound right to you?

BrettSheleski commented 7 years ago

Good catch. That can happen if the edl file starts with a zero indicating the first part of the input file is a commercial (and to be cut out). The indexing variable $i gets incremented before it checks for that resulting in $i being 2 during the next iteration of the loop (where there is content to include in the resulting file).

I'll add a fix for that. And also include chapter titles in the metadata in the format of "chapter $i".

arrmo commented 7 years ago

Thanks!!!