Comskip Initial release #1900

schumi2004 · 2015-10-27T15:13:34Z

Thanks for all the help, it all seems to compile just fine now.
I think it needs to be stripped more because size is around 100MB now.

cytec · 2015-10-27T15:35:11Z

just left you some commits, maybe @Dr-Bean has anything else but besides that. Nice!
Like i mentioned in one of the commits, you may want to read about PLIST creation here: https://github.com/SynoCommunity/spksrc/wiki/PLIST-files

schumi2004 · 2015-10-27T16:00:48Z

Okay, lots of things to work on. Will check them out later today. Thanks

cytec · 2015-10-28T10:31:13Z

spk/comskip/src/dsm-control.sh

+        exit 0
+        ;;
+    log)
+        echo "${INSTALL_DIR}/install.log"


does that file even exist?

Nope ;) If Comskip doesn't maintain it's own log to link to from here, this can be removed.

I thought this was needed for the package itself (install.log) Comskip creates a logfile next to the processed file thus this can be removed indeed.

~~You may have used Mono as an example? It wrote that information during postinstall. That should be removed with the next release, I actually intended to do that and forgot.~~ Actually, I did remove it ;)
Then you may have used the Python package as template. It writes install.log during postinstall here: https://github.com/SynoCommunity/spksrc/blob/master/spk/python/src/installer.sh#L28.

cytec · 2015-10-28T10:33:00Z

except of that one last commit it looks good to me... let's see if @Dr-Bean has anything left :)

Dr-Bean · 2015-10-28T11:33:37Z

spk/comskip/Makefile

+SPK_NAME = comskip
+SPK_VERS = 0.81.089
+SPK_REV = 1
+SPK_ICON = src/comskip.gif


Could you make this a PNG file please? And the file itself doesn't seem to be 256*256, but that would be the recommended size. I can't see the icon right now, but iirc, it was fairly low quality. Maybe take a look at that too.

Dr-Bean · 2015-10-28T11:37:23Z

@Dr-Bean sure has ;) A couple other points: please check that digests are added (make digests).
Besides that, how is comskip used? Is the idea to start it from the command line by itself and use a video file as argument, or does it integrate in other packages somehow? That might be useful to describe somewhere. Maybe a short line in the description (Comskip is a free mpeg commercial detector. It can be used via the command line: start it ascomskipwith the video file as argument., or something like that).
With that, you could consider adding a symlink to /usr/local/bin, similar to nano or other CLI-based packages.

Sorry, wrong button there :/

schumi2004 · 2015-10-28T11:40:55Z

Yeah i was thinking about the same regarding a short howto description. You could schedule this and launch from cli but the way i'm using it is with TVH, when a recording is finished i trigger comskip to process recording and generate a EDL file which can be used in Kodi. More info about this here http://kodi.wiki/view/Edit_decision_list

Will fix other comments also and i think I need to create a new icon since i couldn't find a proper one also.

Dr-Bean · 2015-10-28T11:44:33Z

Depending on how long a howto could be (describing various methods to integrate it?), you could consider writing a wiki page, and add a link like this: https://github.com/SynoCommunity/spksrc/blob/master/spk/ffsync/Makefile#L23. It'll show up as an additional link in Package Center for the package.
You'd want to keep the description field itself fairly limited though, no long stories in there.

Dr-Bean · 2015-10-28T11:46:26Z

Another one: I believe you're missing BETA =1 in the SPK, or did I miss that?

schumi2004 · 2015-10-28T11:50:39Z

Beta isn't there, will add.

Can we use 2 HELPURL's and is it allowed to link to program manual page or should it be hosted on synocommunity wiki?
/edit:
Nevermind, i'll write a wiki page ;)

Dr-Bean · 2015-10-28T11:55:57Z

spk/comskip/src/comskip.ini

+skip_b_frames=0				; Set to 1 to force Comskip to skip frames for higher processing speed.
+hardware_decode=0			; Set to 1 to enable hardware accelerated video decoding, only available in donator version
+max_repair_size=200			; Will repair maximum 200 missing MPEG frames in the timeline, set to 0 to disable repairing for players that don't use PTS. 
+disable_heuristics=4		bit pattern for disabling heuristics, adding 1 disables heristics 1, adding 2 disables heristics 2, adding 4 disables heristics 3, 255  disables all heuristics 


Is this perhaps missing a ; in front of bit pattern [..]?

Yes it is. Fixed it.

Dr-Bean · 2015-10-28T12:06:05Z

You can use SUPPORTURL (official documentation) in your Makefile, along with HELPURL (wiki).

schumi2004 · 2015-10-28T13:31:30Z

Updated and squashed. Image is still bad but better then nothing. Wiki i will update later today.

/edit
Just noticed i forgot the symlink part.

cytec · 2015-10-28T13:43:06Z

spk/comskip/Makefile

+
+DEPENDS = cross/argtable cross/ffmpeg cross/$(SPK_NAME)
+
+RELOAD_UI = yes


seems like you missed to remove this ;)

Wasn't sure if it really should be removed or not.

Will remove it.

Removing this brakes compiling the package, DEPENDS should stay i think?

cytec · 2015-10-28T14:04:01Z

how about taking this as image?

schumi2004 · 2015-10-28T14:09:00Z

I commented on a outdated comment so not sure it it will popup.
But removing this from make file brakes compiling

+SPK_NAME = comskip
+SPK_VERS = 0.81.089
+SPK_REV = 1
+SPK_ICON = src/comskip.png
+BETA = 1
+
+DEPENDS = cross/argtable cross/ffmpeg cross/$(SPK_NAME)
+
+RELOAD_UI = yes

DEPENDS should stay i think?
BETA should also stay imo?

Thanks for the image btw @cytec

Dr-Bean · 2015-10-28T14:10:56Z

You only want to remove RELOAD_UI = yes, not the lines above it. (see schumi2004@23dc962, that commit removed the first 9 SPK-related lines)

Dr-Bean · 2015-10-28T14:12:06Z

@cytec Is that image 256*256? Seems smaller, but that might be my browser. It looks better though, no doubt about that :)

schumi2004 · 2015-10-30T08:29:38Z

I needed to move comskip.ini into bin folder otherwise comskip couldn't find it without adding argument to ini file itself which isn't user friendly either in my opinion.
Also noticed after looking at ffmpeg that the PLIST was different then described in wiki. Not sure if it's needed for ffmpeg but just noticed it. (bin: lnk: man: etc)
l.s.: Am i correct ffmpeg doesn't work for evansport architecture?

Regarding including ffmpeg or not i'll leave it this way, it works now and I can always optimize it later.

/edit:
Wiki is updated. Should get people started.

Dr-Bean · 2015-10-30T08:37:58Z

Hm. I'm not sure I'm willing to publish 80MB*8 arches or so, if we can get away with about half that with a little work. Secondly, the ini file doesn't belong in /bin, and that is going to cause issues with package upgrades. You need to add arguments to comskip anyway, might as well add an ini location. If people don't, I assume comskip is going to complain, or use a set of (incorrect) defaults, yes? (as for an explanation why that would be more acceptable than LD_LIBRARY_PATH, that's because the latter is not obvious. Comskip will tell you that you can set an ini location via comskip --help, but it won't tell you how to find or set ffmpeg libs)

As for ffmpeg: the package needs work, which is what #1662 is for.

schumi2004 · 2015-10-30T08:43:44Z

Comskip is indeed going to complain if it can't find the ini file. (and will use default/incorrect values)
I checked code if I could patch it so it would search in correct directory but haven't find the correct place yet.
When ini is placed next to binary you don't need to submit arguments because comskip finds the ini file itself so you can just use comskip somevideofile.mkv otherwise it would be like this comskip /volume1/@appstore/comskip/var/comskip.ini somevideofile.mkv

Maybe a symlink works (if allowed in packages)?

cytec · 2015-10-30T10:05:33Z

i've checked and it seems like comskip searches your PATH for comskip.ini by default. Not 100% sure but i guess if you want to patch it, you'll have to do it somewhere around here: https://github.com/erikkaashoek/Comskip/blob/8a4d502b0349d9e39179def86a2f681317f76cbc/comskip.c#L12239-L12247 to also check if the file is present in the packages var folder...

cytec · 2015-10-30T10:18:34Z

I've just compiled comskip and removed ffmpeg libs and as i expected it failed running as it needs libavutil on runtime... maybe you can use an approach i used a while ago for a different package:

Compile it with cross/ffmpeg and then in the Makefile of the spk remove everything not needed... not really the cleanest solution but at least you'll not end up with an 80MB Package... maybe this works :) for testing purpose you could just try to remove some libs from the lib folder and check if it still runs ...

schumi2004 · 2015-10-30T10:37:02Z

Maybe not cleanest but works for me, good idea though.
At first i'm going to fix myself a decent demo file to test.
After that i'm going to manually remove stuff and see how it goes.

Also reverted commit for putting ini file in bin folder, it's back in var again (with extra comskip.dictionary file)
Users should always give full path to ini file now until i found a solution.
I noticed those lines in comskip.c also but thought it was for windows builds. I asked someone at Kodi forum if he could help me with this PATH thing.

cytec · 2015-10-30T10:44:47Z

you could also just append /usr/local/comspkip/var to the PATH ... maybe putenv would to the trick?

so that would look something like this i think: putenv('PATH=/usr/local/comskip/var/:$PATH') where $PATH is what is currently in your PATH already ;)

Diaoul · 2015-10-30T11:04:46Z

The cleanest solution for this is to patch comskip to accept an extra directory to search for the .ini defined in an env variable.
When you compile comskip SPK you set that compile-time env variable to the /usr/local/comspkip/var directory. You will end up with a solution that doesn't require any user action and is still flexible enough so you can change that directory without changing the patch file (not hardcoded).

cytec · 2015-10-30T11:22:23Z

+1 for this solution... maybe you'll also append /usr/local/ffmpeg/lib/ to the LD_LIBRARY_PATH on runtime compilation so that you are able to move ffmpeg to BUILD_DEPENDS and add ffmpeg SPK to the SPK depends...

seems like it needs more than just libavutil :/

EDIT: maybe setting an --rpath would work also?

Diaoul · 2015-10-30T12:03:14Z

+1 for --rpath or reusing something existing rather than patching to add a library path.

schumi2004 · 2015-10-30T12:26:28Z

I need to figure out the append stuff and BUILD and SPK depends and also --rpath (all new for me)
Hopefully I can fix and test it today.

cytec · 2015-10-30T12:36:20Z

i'm not 100% sure but if i remember correctly setting --rpath can be done by useing ADDITIONAL_LDFLAGS like this: ADDITIONAL_LDFLAGS="-Wl,-rpath,/usr/local/ffmpeg/lib/"

@Dr-Bean @Diaoul correct me if im wrong ;)

for the env patch for comskip: maybe @Diaoul can provide some more help/information on this...

tmm1 · 2015-10-31T15:36:21Z

The PATH searching code is only enabled on windows. I don't mind adding it for other platforms but we will need a posix implementation of _searchenv in platform.c

schumi2004 · 2015-11-03T07:34:48Z

@tmm1 adding PATH for other environments would make things alot easier but don't know how easy/hard it is to implement.

schumi2004 · 2015-12-18T08:09:43Z

@cytec
Can you provide me source code to compile ffmpeg for Evansport architecture? I spoke to someone that said he got a Evansport package from you?

cytec · 2015-12-18T09:17:23Z

@schumi2004 should be the source from here https://github.com/cytec/spksrc/tree/ffmpeg-update

schumi2004 · 2015-12-19T02:50:33Z

@cytec
I compiled ffmpeg after i cloned your fork but it returns an error, any ideas?

libopenjpeg/jpwl/Makefile.am:5: warning: source file '../tpix_manager.c' is in a subdirectory,
libopenjpeg/jpwl/Makefile.am:5: but option 'subdir-objects' is disabled
autoreconf: automake failed with exit status: 1
Makefile:21: recipe for target 'myPreconf' failed
make[1]: *** [myPreconf] Error 1
make[1]: Leaving directory '/home/openelec/github/cytec-spksrc/cross/openjpeg'
../../mk/spksrc.cross-cc.mk:117: recipe for target 'arch-evansport-5.1' failed
make: [arch-evansport-5.1] Error 2 (ignored)

/EDIT:
It seems i was missing a mandatory package, issue solved.

schumi2004 · 2015-12-19T09:39:25Z

@cytec could you have a look at rtmpdump? (Well, i think it's rtmpdump, not really sure)
I'm unable to compile it, is it still needed?

sr/local/ffmpeg/lib -Wl,--rpath-link,/home/openelec/github/cytec-spksrc/spk/ffmpeg/work-evansport-5.2/install//usr/local/ffmpeg/lib -Wl,--rpath,/usr/local/ffmpeg/lib "
/bin/sh: 1: cd: can't cd to /home/openelec/github/cytec-spksrc/spk/ffmpeg/work-evansport-5.2/rtmpdump-ksv
Makefile:20: recipe for target 'myCompile' failed
make[3]: *** [myCompile] Error 2
make[3]: Leaving directory '/home/openelec/github/cytec-spksrc/cross/rtmpdump'
../../mk/spksrc.depend.mk:44: recipe for target 'depend_target' failed
make[2]: *** [depend_target] Error 2
make[2]: Leaving directory '/home/openelec/github/cytec-spksrc/cross/ffmpeg'
../../mk/spksrc.depend.mk:44: recipe for target 'depend_target' failed
make[1]: *** [depend_target] Error 2
make[1]: Leaving directory '/home/openelec/github/cytec-spksrc/spk/ffmpeg'
../../mk/spksrc.spk.mk:417: recipe for target 'arch-evansport-5.2' failed
make: [arch-evansport-5.2] Error 2 (ignored)

/EDIT:
It seems more is broken since recent commits, now i'm unable to compile ffmpeg in general
CC libavdevice/v4l2-common.o
CC libavdevice/v4l2.o
CC libavdevice/v4l2enc.o
AR libavdevice/libavdevice.a
/home/openelec/github/spksrc/toolchains/syno-alpine-5.2/work/arm-cortex_a15-linux-gnueabi/bin/arm-cortex_a15-linux-gnueabi-ranlib: '-D': No such file
library.mak:39: recipe for target 'libavdevice/libavdevice.a' failed
make[2]: *** [libavdevice/libavdevice.a] Error 1
make[2]: Leaving directory '/home/openelec/github/spksrc/cross/ffmpeg/work-alpine-5.2/ffmpeg-2.6'
../../mk/spksrc.compile.mk:36: recipe for target 'compile_target' failed
make[1]: *** [compile_target] Error 2
make[1]: Leaving directory '/home/openelec/github/spksrc/cross/ffmpeg'
../../mk/spksrc.cross-cc.mk:122: recipe for target 'arch-alpine-5.2' failed
make: [arch-alpine-5.2] Error 2 (ignored)
12:19:32 (comskip) ~/github/spksrc/cross/ffmpeg$

schumi2004 · 2016-01-21T18:03:36Z

@cytec
I think this worked ADDITIONAL_LDFLAGS="-Wl,-rpath,/usr/local/ffmpeg/lib/"
(and removing bin files for ffmpeg)
I have a package that is 2,4MB in size now.

Will test ffmpeg package and see how it works on test NAS

schumi2004 · 2016-05-11T09:12:58Z

Comskip depends on ffmpeg-3.x these days. Since ffmpeg-3.x isn't in SynoCommunity's repo yet this PR can't be merged.

cytec · 2016-05-12T15:10:15Z

yeah... i should really get my ffmpeg stuff merged -.-

schumi2004 · 2016-05-13T09:03:37Z

I have a ffmpeg-3.0.2 package available here. I could submit it so you can review?

ndaversa · 2016-11-15T03:16:56Z

@schumi2004 with this landing: #2375 can we get comskip landed as well?

cc: @cytec

schumi2004 · 2016-11-15T07:14:39Z

I'll finish it today ;)

schumi2004 · 2016-11-16T21:27:55Z

Current ffmpeg isn't working by default.
Need to figure out what is different with mine.

schumi2004 mentioned this pull request Oct 27, 2015

Comskip #380

Closed

cytec added the status/needs-review label Oct 27, 2015

cytec reviewed Oct 28, 2015
View reviewed changes

Dr-Bean reviewed Oct 28, 2015
View reviewed changes

Dr-Bean closed this Oct 28, 2015

Dr-Bean reopened this Oct 28, 2015

Dr-Bean reviewed Oct 28, 2015
View reviewed changes

schumi2004 force-pushed the comskip branch from 7527823 to deb8140 Compare October 28, 2015 13:30

schumi2004 force-pushed the comskip branch from deb8140 to a09cdd9 Compare October 28, 2015 13:35

cytec reviewed Oct 28, 2015
View reviewed changes

schumi2004 force-pushed the comskip branch from 053b67a to a719a63 Compare October 30, 2015 10:24

schumi2004 closed this Feb 17, 2017

schumi2004 force-pushed the comskip branch from a719a63 to 7a45b96 Compare February 17, 2017 22:24

schumi2004 mentioned this pull request Feb 17, 2017

Comskip initial release #2673

Merged

m4tt075 mentioned this pull request Aug 9, 2017

Update Chromaprint to 1.3.2 and ffmpeg to 3.3.3 and dependencies #2865

Merged

3 tasks


		DEPENDS = cross/argtable cross/ffmpeg cross/$(SPK_NAME)

		RELOAD_UI = yes

Comskip Initial release #1900

Comskip Initial release #1900

Conversation

schumi2004 commented Oct 27, 2015

cytec commented Oct 27, 2015

schumi2004 commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cytec commented Oct 28, 2015

Choose a reason for hiding this comment

Dr-Bean commented Oct 28, 2015

schumi2004 commented Oct 28, 2015

Dr-Bean commented Oct 28, 2015

Dr-Bean commented Oct 28, 2015

schumi2004 commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dr-Bean commented Oct 28, 2015

schumi2004 commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cytec commented Oct 28, 2015

schumi2004 commented Oct 28, 2015

Dr-Bean commented Oct 28, 2015

Dr-Bean commented Oct 28, 2015

schumi2004 commented Oct 30, 2015

Dr-Bean commented Oct 30, 2015

schumi2004 commented Oct 30, 2015

cytec commented Oct 30, 2015

cytec commented Oct 30, 2015

schumi2004 commented Oct 30, 2015

cytec commented Oct 30, 2015

Diaoul commented Oct 30, 2015

cytec commented Oct 30, 2015

Diaoul commented Oct 30, 2015

schumi2004 commented Oct 30, 2015

cytec commented Oct 30, 2015

tmm1 commented Oct 31, 2015

schumi2004 commented Nov 3, 2015

schumi2004 commented Dec 18, 2015

cytec commented Dec 18, 2015

schumi2004 commented Dec 19, 2015

schumi2004 commented Dec 19, 2015

schumi2004 commented Jan 21, 2016

schumi2004 commented May 11, 2016

cytec commented May 12, 2016

schumi2004 commented May 13, 2016

ndaversa commented Nov 15, 2016 • edited Loading

schumi2004 commented Nov 15, 2016

schumi2004 commented Nov 16, 2016

ndaversa commented Nov 15, 2016 •

edited

Loading