Discussion:
Request for review: A Common Lisp package to slice, splice, and combine media files
(too old to reply)
Udyant Wig
2020-06-28 12:42:30 UTC
Permalink
I would like to offer a project of mine for review:
<URL:https://gitlab.com/udyant/slice-media>

'A simple Common Lisp package to slice, splice, and combine media
files.'

Fundamentally, it is an interface to control ffmpeg:
<URL:https://ffmpeg.org/>

My development system is SLIME on Linux; I have not tested on other
platforms such as Windows and macOS.


I use it most frequently to slice video files. The workflow follows.

(1) View the source video, note start and finish times of each desired
section.

(2) Prepare a Lisp file with the form

(((<sh 1> <sm 1> <ss 1>) (<eh 1> <em 1> <es 1>))
((<sh 2> <sm 2> <ss 2>) (<eh 2> <em 2> <es 2>))
...
((<sh n> <sm n> <ss n>) (<eh n> <em n> <es n>)))

where sh := start hour
sm := start minute
ss :> start second
eh := end hour
em := end minute
es := end second

There must be as many such timing pairs as the number of desired
sections of the video.

(3) Call the function SLICE-MEDIA with these required keyword arguments
:SOURCE-FILE, :DESTINATION-FILE, and TIMINGS-FILE.

(4) Wait for SLICE-MEDIA to exit with return value T.

(5) Check the resultant video.

(6) Alter timings and repeat if necessary.


There is also a provision to speed up or slow down a given video.


Udyant Wig
--
We make our discoveries through our mistakes: we watch one another's
success: and where there is freedom to experiment there is hope to
improve.
-- Arthur Quiller-Couch
Udyant Wig
2020-06-30 20:09:11 UTC
Permalink
The project is now publicly accessible. I had forgotten to allow that.

Mea culpa.

Udyant Wig
--
We make our discoveries through our mistakes: we watch one another's
success: and where there is freedom to experiment there is hope to
improve.
-- Arthur Quiller-Couch
Lieven Marchand
2020-06-30 21:13:52 UTC
Permalink
Post by Udyant Wig
The project is now publicly accessible. I had forgotten to allow that.
Mea culpa.
Looks ok. A few small nits:

(defun seconds-as-time-slice (secs)
"Make a time-slice out of the given number of seconds, SECS."
(let ((hours (prog1
(floor (/ secs 3600))
(setf secs (mod secs 3600))))
(minutes (prog1
(floor (/ secs 60))
(setf secs (mod secs 60))))
(seconds secs))
(make-time-slice :hours hours :minutes minutes :seconds seconds)))

FLOOR has a 2 argument version that already gives you the remainder as
second return value so it's a tad compacter to do

; untested but you get the idea
(multiple-value-bind (hours rsecs)
(floor secs 3600)
(multiple-value-bind (minutes rrsecs)
(floor rsecs 60)
(make-time-slice :hours hours :minutes minutes :seconds rrsecs)))

In make-time-slice-pairs you can just do "for i from 0". The first for
will end the iteration. Also you can replace the push/nreverse by
collect.

For read-triplet-pairs, the usual caveat about using the lisp reader to
parse user input goes. At the very least, use WITH-STANDARD-IO-SYNTAX
and bind *READ-EVAL*. You wouldn't want someone to put
#.(sys:format-disks) into an input file.

all-t-p can be done with EVERY.

(defun cut-slices (time-slice-pairs)
"Execute *FFMPEG-PROGRAM* to cut intermediate slices out of *SOURCE-FILE* per TIME-SLICE-PAIRS."
(loop with slice-cut-results = ()
with slice-cut-p = nil
for time-slice-pair in time-slice-pairs
do
(setf slice-cut-p (cut-slice time-slice-pair))
(push slice-cut-p slice-cut-results)
finally (return (all-t-p slice-cut-results))))

I like LOOP too but this can be done as a one liner

(every #'identity (mapcar #'cut-slice time-slice-pairs))

In general, look at the accumulation clauses of LOOP. A lot of your
(loop with var do <modify var> finally return var) patterns can be done
with them.

But over all the code looks fine. These are just personal preferences.
--
Laat hulle almal sterf. Ek is tevrede om die wêreld te sien brand en die vallende
konings te spot. Ek en my aasdier sal loop op die as van die verwoeste aarde.
Udyant Wig
2020-07-01 06:45:40 UTC
Permalink
Post by Lieven Marchand
(defun seconds-as-time-slice (secs)
"Make a time-slice out of the given number of seconds, SECS."
(let ((hours (prog1
(floor (/ secs 3600))
(setf secs (mod secs 3600))))
(minutes (prog1
(floor (/ secs 60))
(setf secs (mod secs 60))))
(seconds secs))
(make-time-slice :hours hours :minutes minutes :seconds seconds)))
FLOOR has a 2 argument version that already gives you the remainder as
second return value so it's a tad compacter to do
; untested but you get the idea
(multiple-value-bind (hours rsecs)
(floor secs 3600)
(multiple-value-bind (minutes rrsecs)
(floor rsecs 60)
(make-time-slice :hours hours :minutes minutes :seconds rrsecs)))
Yes, this is much clearer.
Post by Lieven Marchand
In make-time-slice-pairs you can just do "for i from 0". The first for
will end the iteration. Also you can replace the push/nreverse by
collect.
Noted.
Post by Lieven Marchand
For read-triplet-pairs, the usual caveat about using the lisp reader to
parse user input goes. At the very least, use WITH-STANDARD-IO-SYNTAX
and bind *READ-EVAL*. You wouldn't want someone to put
#.(sys:format-disks) into an input file.
Indeed not.
Post by Lieven Marchand
all-t-p can be done with EVERY.
Okay.
Post by Lieven Marchand
(defun cut-slices (time-slice-pairs)
"Execute *FFMPEG-PROGRAM* to cut intermediate slices out of *SOURCE-FILE* per TIME-SLICE-PAIRS."
(loop with slice-cut-results = ()
with slice-cut-p = nil
for time-slice-pair in time-slice-pairs
do
(setf slice-cut-p (cut-slice time-slice-pair))
(push slice-cut-p slice-cut-results)
finally (return (all-t-p slice-cut-results))))
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Oh dear. It seems I have come be enamored of LOOP without understanding
it better.
Post by Lieven Marchand
In general, look at the accumulation clauses of LOOP. A lot of your
(loop with var do <modify var> finally return var) patterns can be
done with them.
Yes. I see that now.
Post by Lieven Marchand
But over all the code looks fine. These are just personal preferences.
I appreciate your looking it over. Thank you very much.

Udyant Wig
--
We make our discoveries through our mistakes: we watch one another's
success: and where there is freedom to experiment there is hope to
improve.
-- Arthur Quiller-Couch
t***@google.com
2020-07-01 15:47:14 UTC
Permalink
Post by Lieven Marchand
For read-triplet-pairs, the usual caveat about using the lisp reader to
parse user input goes. At the very least, use WITH-STANDARD-IO-SYNTAX
and bind *READ-EVAL*. You wouldn't want someone to put
#.(sys:format-disks) into an input file.
all-t-p can be done with EVERY.
(defun cut-slices (time-slice-pairs)
"Execute *FFMPEG-PROGRAM* to cut intermediate slices out of *SOURCE-FILE* per TIME-SLICE-PAIRS."
(loop with slice-cut-results = ()
with slice-cut-p = nil
for time-slice-pair in time-slice-pairs
do
(setf slice-cut-p (cut-slice time-slice-pair))
(push slice-cut-p slice-cut-results)
finally (return (all-t-p slice-cut-results))))
The loop can be simplified a bit to:

(loop for time-slice-pair in time-slice-pairs
collect (cut-slice time_slice-pair) into slice-cut-results
finally (return (all-t-p slice-cut-results)))

And one might also consider the following return if you want the results as well as just the boolean test applied to the results.
(return (values (all-t-p slice-cut-results) slice-cut-results))
Post by Lieven Marchand
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Although a big LOOP fan, I do like the simplicity of this formulation.
Stefan Monnier
2020-07-01 16:52:55 UTC
Permalink
Post by Lieven Marchand
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Isn't this just (every #'cut-slice time-slice-pairs)?


Stefan
Lieven Marchand
2020-07-01 17:52:46 UTC
Permalink
Post by Stefan Monnier
Post by Lieven Marchand
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Isn't this just (every #'cut-slice time-slice-pairs)?
No. EVERY stops at the first false result. The original loops over the
entire list of slices and then checks whether any failed.
--
Laat hulle almal sterf. Ek is tevrede om die wêreld te sien brand en die vallende
konings te spot. Ek en my aasdier sal loop op die as van die verwoeste aarde.
t***@google.com
2020-07-01 19:50:42 UTC
Permalink
Post by Lieven Marchand
Post by Stefan Monnier
Post by Lieven Marchand
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Isn't this just (every #'cut-slice time-slice-pairs)?
No. EVERY stops at the first false result. The original loops over the
entire list of slices and then checks whether any failed.
That is correct.
But since CUT-SLICE doesn't have any side effects, it shouldn't matter if they
all get tested or not. The function doesn't return the number of failures or
the set of returned slices. So the value of calling the function will be the
same either way.
Udyant Wig
2020-07-03 06:26:26 UTC
Permalink
Post by Lieven Marchand
Post by Stefan Monnier
Post by Lieven Marchand
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Isn't this just (every #'cut-slice time-slice-pairs)?
No. EVERY stops at the first false result. The original loops over the
entire list of slices and then checks whether any failed.
Now that I think about it having seen it recast in terms of EVERY, it
probably makes much more sense to stop if a slice could not be cut (for
whatever reason), rather than proceeding with the rest of them.

Thanks for the discussion that shined a new light on this.

Udyant Wig
--
We make our discoveries through our mistakes: we watch one another's
success: and where there is freedom to experiment there is hope to
improve.
-- Arthur Quiller-Couch
t***@google.com
2020-07-03 21:54:46 UTC
Permalink
Post by Udyant Wig
Post by Lieven Marchand
Post by Stefan Monnier
Post by Lieven Marchand
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Isn't this just (every #'cut-slice time-slice-pairs)?
No. EVERY stops at the first false result. The original loops over the
entire list of slices and then checks whether any failed.
Now that I think about it having seen it recast in terms of EVERY, it
probably makes much more sense to stop if a slice could not be cut (for
whatever reason), rather than proceeding with the rest of them.
But is the result of cutting the slices only supposed to be a true/false value
of whether the slices could be cut?

Or do you want to have the list of slices if it were possible to create all of
them?
Kaz Kylheku
2020-07-04 02:18:33 UTC
Permalink
Post by t***@google.com
Post by Udyant Wig
Post by Lieven Marchand
Post by Stefan Monnier
Post by Lieven Marchand
I like LOOP too but this can be done as a one liner
(every #'identity (mapcar #'cut-slice time-slice-pairs))
Isn't this just (every #'cut-slice time-slice-pairs)?
No. EVERY stops at the first false result. The original loops over the
entire list of slices and then checks whether any failed.
Now that I think about it having seen it recast in terms of EVERY, it
probably makes much more sense to stop if a slice could not be cut (for
whatever reason), rather than proceeding with the rest of them.
But is the result of cutting the slices only supposed to be a true/false value
of whether the slices could be cut?
Or do you want to have the list of slices if it were possible to create all of
them?
We have the list of slices: it's in the variable time-slice-pairs.

The every call is just side-effecting upon them with short-circuiting
semantics, and a summarized result value (were all the effects done or
not).
Udyant Wig
2020-07-04 14:42:55 UTC
Permalink
Post by Kaz Kylheku
Post by t***@google.com
But is the result of cutting the slices only supposed to be a
true/false value of whether the slices could be cut?
Or do you want to have the list of slices if it were possible to
create all of them?
We have the list of slices: it's in the variable time-slice-pairs.
The every call is just side-effecting upon them with short-circuiting
semantics, and a summarized result value (were all the effects done or
not).
In a situation like this, where I obtain a bad end result, I try to
retain the intermediate slices to see what went badly. (Done by the
keyword argument :keep-intermediate-files to SLICE-MEDIA.)

Would it useful to have CUT-SLICE put forward a condition to let its
callers know something about the affected slice?

Udyant Wig
--
We make our discoveries through our mistakes: we watch one another's
success: and where there is freedom to experiment there is hope to
improve.
-- Arthur Quiller-Couch
Loading...