search

BrettSheleski / comchap

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

Tweaks to comchap #37

Open heegard opened 5 years ago

heegard commented 5 years ago

I've made a few tweaks to comchap, including:

  1. Improved Usage (i.e., help)
  2. Make end at least 100 (helps with initial segment of video)
  3. Added option time-marks to title.

-Chris

BrettSheleski commented 5 years ago

What do you mean that you make end at least 100?

heegard commented 5 years ago

Making "end" at least 100 makes sure the initial segment has a positive, but small, length... I found it helped for a better sequence of segments. -Chris

BrettSheleski commented 5 years ago

I'm not sure how I feel about this to be honest.

On one hand I feel that this could be considered an issue with ComSkip. The duty of Comchap/Comcut is to perform the actions directed to it by the edl file. If the edl indicates very short chapters, then very short chapters the file should have. Is it specific players' playback that is an issue? Then perhaps it's an issue with the player (again: not Comchap/Comcut).

On the other hand I want to make a tool people actually use and find useful. With this in mind I'd like to include this feature.

However I do not like how it automatically adjusts the end time by some seemingly arbitrary amount. Instead I would suggest taking in this minimum duration as a command line argument allowing the user to specify their own minimum.

Also, it looks like this would result in chapters with 100ms durations. Having 100ms duration chapters doesn't seem very useful to me. In the case of $end being less than 100 (or the user-specified minimum mentioned above) instead of making a chapter 100ms, the content should instead be included into the previous chapter.

If you implement the following, I'll gladly accept your pull request.

BrettSheleski commented 5 years ago

I also do not like that a dependency to perl has been added just to make the format of the chapter titles to your liking. I'm not opposed to changing the format of the title of the chapters (I personally don't find them useful anyway), but I really want to avoid adding another dependency to the project.

heegard commented 5 years ago
  1. I'm fine with removing the code that sets $end to a minimum of 100. It only affects the first 1/10 of a second of the video, so it's not a big deal.
  2. The perl code is pure perl (no libraries); I cannot think of a system that includes bash (and awk/sed) that doesn't support perl.
  3. I added the time-mark because for a while plex didn't display the time mark of chapters, so it was helpful to figure out where you were in order to skip the commericials. Plex has now re-introduced the time marks, so it is less useful.
  4. The option description for help is useful.

I can adjust these things to your liking... I find comchap useful and thought I could add to it.

-Chris

mjbrowns commented 5 years ago

FYI - I run comskip in a docker container environment where my post processor that uses comskip does not have perl installed.

I will try to look at these changes and see if they can be done without perl

Regards,

Mitch Brown mitch@mjbrowns.com 330-391-0447

From: Gatomon notifications@github.com Sent: Saturday, June 8, 2019 12:15 AM To: BrettSheleski/comchap Cc: Subscribed Subject: Re: [BrettSheleski/comchap] Tweaks to comchap (#37)

  1. I'm fine with removing the code that sets $end to a minimum of 100. It only affects the first 1/10 of a second of the video, so it's not a big deal.
  2. The perl code is pure perl (no libraries); I cannot think of a system that includes bash (and awk/sed) that doesn't support perl.
  3. I added the time-mark because for a while plex didn't display the time mark of chapters, so it was helpful to figure out where you were in order to skip the commericials. Plex has now re-introduced the time marks, so it is less useful.
  4. The option description for help is useful.

I can adjust these things to your liking... I find comchap useful and thought I could add to it.

-Chris

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FBrettSheleski%2Fcomchap%2Fpull%2F37%3Femail_source%3Dnotifications%26email_token%3DAGHZPMQROSOZLKHPH4XFTS3PZMW5PA5CNFSM4HUVL4PKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHM34I%23issuecomment-500092401&data=02%7C01%7C%7Cf4d3687dfe2841bf421708d6ebc7fba1%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636955641480079010&sdata=wVisv08Dx7eQfGd3NQkhi73ve9XzirJ9x2ekXp1Uwq4%3D&reserved=0, or mute the threadhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAGHZPMTWWXW63QRNXKG2QLDPZMW5PANCNFSM4HUVL4PA&data=02%7C01%7C%7Cf4d3687dfe2841bf421708d6ebc7fba1%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636955641480089015&sdata=tUhcG92gs79PCB5A85yuOpOiSCbjn77gQ9lQmfwL0HI%3D&reserved=0.

heegard commented 5 years ago

Mitch,

That sounds good to me.

-Chris