Skip to content

Adder timer function and option for filename #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Dark-Kernel wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Adder timer function and option for filename #39

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
03bdd0a
Developed -o option for output filename & included DIR for file path …
Dark-Kernel Jul 13, 2024
94941f0
Added timer to recording.
Dark-Kernel Jul 13, 2024
d7a75f7
Ensure writing config clobbers old config
belden Jul 15, 2024
667c064
Whitespace cleanup
belden Jul 15, 2024
1c4f76d
Merge pull request #1 from belden/pr-tidy
Dark-Kernel Jul 16, 2024
78bf462
changed exit status back to -1, Default dir o desktop. Added usage f…
Dark-Kernel Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 91 additions & 51 deletions bin/ffscreencast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ TIME=$(date +%H.%M.%S)
NAME="Screencast ${DATE} at ${TIME}"

# Where to save it
DIR="${HOME}/Desktop"
DIR="${HOME}/Videos/"
Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dark-Kernel Is this a mistaken commit? I imagine it might be left over from your development, since it's not called out in the PR description.

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but, No it's not mistaken. Is there something wrong?

Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be very clear: I think this is a good change to make, and might resolve #34 (or might not, perhaps that user doesn't keep a ~/Videos/ either).

This is a change in prior behavior from how the tool used to operate. It might be worth making a separate PR just for changing the default save directory, so @cytopia can either approve or reject it as they see fit, independent of the timer functionality your PR is adding.

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Videos directory is most likely available on all Desktop Linux distributions, and if it's not there that means most probably that user will neither have Desktop directory. (Like me)
BTW thanks.




Expand Down Expand Up @@ -124,6 +124,32 @@ EXIT_OK="0"
################################################################################


###############################################################################
# Timer Funtion
###############################################################################

# Main function to run the timer
run_timer() {
start_time=$SECONDS
# display_timer="$(echo -ne "$(date -u --date @$((SECONDS - start_time)) +%H:%M:%S)\r")"
while true; do
echo -ne "$(date -u --date @$((SECONDS - start_time)) +%H:%M:%S)\r"
sleep 1
done
}

# Trap CTRL+C to stop the timer and exit
trap_ctrlc() {
echo -e "\nRecording stopped."
exit
}

# Set up trap to catch CTRL+C and call trap_ctrlc function
trap trap_ctrlc INT

# Main script starts here
echo "Press CTRL+C to stop the recording."

################################################################################
# Helper Function
################################################################################
Expand Down Expand Up @@ -153,52 +179,58 @@ write_config() {
$(which mkdir) -p "${dir}"
fi

echo "# ~/.config/ffscreencast/ffscreencastrc" > "${conf}"
echo >> "${conf}"

echo "# Default video container extension" >> "${conf}"
echo "# Alternatively: 'mp4' or 'avi'" >> "${conf}"
echo "OUTPUT_EXT=\"mkv\"" >> "${conf}"
echo >> "${conf}"

echo "# Default audio output codec" >> "${conf}"
echo "# Alternatively: 'pcm_s16le'" >> "${conf}"
echo "OUTPUT_ACODEC=\"libfaac\"" >> "${conf}"
echo >> "${conf}"

echo "# Default video output codec" >> "${conf}"
echo "# Alternatively: 'libx265'" >> "${conf}"
echo "OUTPUT_VCODEC=\"libx264\"" >> "${conf}"
echo >> "${conf}"


echo "# Default Screen recording arguments" >> "${conf}"
echo "S_ARGS=\"\"" >> "${conf}"
echo >> "${conf}"

echo "# Default audio recording arguments" >> "${conf}"
echo "A_ARGS=\"-ac 2\"" >> "${conf}"
echo >> "${conf}"

echo "# Default camera recording arguments" >> "${conf}"
echo "C_ARGS=\"\"" >> "${conf}"
echo >> "${conf}"

echo "# Default misc output arguments" >> "${conf}"
echo "O_ARGS=\"-crf 0 -preset ultrafast\"" >> "${conf}"
echo >> "${conf}"

echo "# Default recording behavior" >> "${conf}"
echo "RECORD_S=\"yes\"" >> "${conf}"
echo "RECORD_A=\"no\"" >> "${conf}"
echo "RECORD_C=\"no\"" >> "${conf}"
echo >> "${conf}"

echo "# What listed device number has been chosen to record?" >> "${conf}"
echo "CHOSEN_S_NUM=\"\"" >> "${conf}"
echo "CHOSEN_A_NUM=\"\"" >> "${conf}"
echo "CHOSEN_C_NUM=\"\"" >> "${conf}"
echo >> "${conf}"
{
echo "# ~/.config/ffscreencast/ffscreencastrc"
echo

echo "# Default Directory to save video files"
echo "DIR=\"$HOME/Videos\""
Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I wonder whether this is an intended change?

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having DIR in configuration, one can easily change it to intended location.

Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to reply here as well, changing default behavior of tools can surprise users. Though I personally think ~/Videos/ is a more sensible default location than ~/Desktop/, I have also gotten used to the existing behavior of the tool.

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, so I think I should change it to ~/Desktop again.

Sorry for the inconvenience.

echo

echo "# Default video container extension"
echo "# Alternatively: 'mp4' or 'avi'"
echo "OUTPUT_EXT=\"mkv\""
echo

echo "# Default audio output codec"
echo "# Alternatively: 'pcm_s16le'"
echo "OUTPUT_ACODEC=\"libfaac\""
echo

echo "# Default video output codec"
echo "# Alternatively: 'libx265'"
echo "OUTPUT_VCODEC=\"libx264\""
echo


echo "# Default Screen recording arguments"
echo "S_ARGS=\"\""
echo

echo "# Default audio recording arguments"
echo "A_ARGS=\"-ac 2\""
echo

echo "# Default camera recording arguments"
echo "C_ARGS=\"\""
echo

echo "# Default misc output arguments"
echo "O_ARGS=\"-crf 0 -preset ultrafast\""
echo

echo "# Default recording behavior"
echo "RECORD_S=\"yes\""
echo "RECORD_A=\"no\""
echo "RECORD_C=\"no\""
echo

echo "# What listed device number has been chosen to record?"
echo "CHOSEN_S_NUM=\"\""
echo "CHOSEN_A_NUM=\"\""
echo "CHOSEN_C_NUM=\"\""
echo
} >> "${conf}"
Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dark-Kernel FYI there's a subtle change in behavior here. Previously passing through this code would do

echo "foo" > $conf # clobber

and then a series of

echo "bar" >> $conf # append new file

Changing this to

{
    echo "foo"
    echo "bar"
} >> $conf

means we no longer clobber the old config, instead we'll append a new config to it.

I think you can just use

{
    echo "foo"
    echo "bar"
} > $conf # note, just a single `>`

to achieve the prior behavior.

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's small mistake, i didn't noticed that one > is extra, sorry 😅.

Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dark-Kernel cool, it seems like an easy fix - but this PR doesn't contain it (yet - maybe you haven't had a chance to make it yet: totally understandable!)

Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened Dark-Kernel#1 which targets your branch - if merged it will resolve this >> -> > issue.

fi

}
Expand All @@ -220,6 +252,7 @@ print_usage() {
printf "%s %s %s\n" " " "${INFO_NAME}" "--help"
printf "%s %s %s\n" " " "${INFO_NAME}" "--version"
printf "%s %s %s\n" " " "${INFO_NAME}" "--test"
printf "%s %s %s\n" " " "${INFO_NAME}" "--output-file"
}

#
Expand Down Expand Up @@ -273,6 +306,8 @@ print_help() {
echo " Specify additional ffmpeg arguments for the output encoding."
echo " Use: --oargs=\"-crf 0\""
echo " Default: '-crf 0 -preset ultrafast'"
echo
echo "-o Output video file name"
echo
echo
echo "Behavior options:"
Expand Down Expand Up @@ -859,6 +894,11 @@ while [ $# -gt 0 ]; do
-e[a-z]*)
OUTPUT_EXT="$(echo "$1" | $SED 's/^..//g')"
;;
-o*)
shift
NAME="$1"
run_timer &
Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, this line is indented with spaces rather than tabs. The file in general uses tabs.

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I saw in git diff, I always use tabs. I don't know how this occurred.

Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened Dark-Kernel#1 which targets your branch - if merged, it will resolve this issue.

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

;;



Expand Down Expand Up @@ -1053,7 +1093,7 @@ if [ "${RECORD_S}" = "yes" ]; then
if [ ${#CHOSEN_S_NUM} -gt 0 ]; then
if ! screen_device_exists "$CHOSEN_S_NUM"; then
echo "Screen recording device: '${CHOSEN_S_NUM}' does not exist."
exit -1
exit 1
Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dark-Kernel I'm curious about this change? Again, it's a slight change, but it's not commented in the PR so I'm uncertain as to the rationale.

$ bash -c 'exit -1'
$ echo $?
255

$ bash -c 'exit 1'
$ echo $?
1

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what difference it makes?

Copy link

@belden belden Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is just me noticing an unacknowledged change in default behavior.

My interest in this project is to be able to easily record my screen on Linux. I use i3wm, which is a very customizable window manager.

Right now it is nice that ffscreencast has three different exit values:

  • 0 -- no errors; my screen recording is where I want it to be
  • 1 -- some sort of error occurred, probably while running
  • 255 -- the script couldn't even start doing its work

Changing from exit -1 to exit 1 here means I get a little less information from the exit value of ffscreencast.

Copy link
Author

@Dark-Kernel Dark-Kernel Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Learned something new. Will change it.

fi
screen_device="$CHOSEN_S_NUM"

Expand All @@ -1076,7 +1116,7 @@ if [ "${RECORD_A}" = "yes" ]; then
if [ ${#CHOSEN_A_NUM} -gt 0 ]; then
if ! audio_device_exists "$CHOSEN_A_NUM"; then
echo "Audio recording device: '${CHOSEN_A_NUM}' does not exist."
exit -1
exit 1
fi
audio_device="$CHOSEN_A_NUM"

Expand All @@ -1099,7 +1139,7 @@ if [ "${RECORD_C}" = "yes" ]; then
if [ ${#CHOSEN_C_NUM} -gt 0 ]; then
if ! camera_device_exists "$CHOSEN_C_NUM"; then
echo "Camera recording device: '${CHOSEN_C_NUM}' does not exist."
exit -1
exit 1
fi
camera_device="$CHOSEN_C_NUM"

Expand Down Expand Up @@ -1183,7 +1223,7 @@ fi


#FFMPEG="ffmpeg"
FFMPEG="${FFMPEG} -hide_banner -loglevel info"
FFMPEG="${FFMPEG} -hide_banner -v quiet"

# Video Options
FFMPEG="${FFMPEG} -thread_queue_size 512"
Expand Down