colinkeenan / silentcast

Create silent mkv screencast and animated gif.
GNU General Public License v3.0
512 stars 22 forks source link

The wizard is quite confusing #32

Open dandv opened 7 years ago

dandv commented 7 years ago

I'm coming from Gif Recorder, which was pulled by the author. Thanks for making Silentcast free!

However, the wizard/setup experience is unnecessarily confusing IMO. There are about 5 screens before you can start recording. The second time I ran bash silentcast there were fewer dialogs, but it was unclear what exactly was being recorded, and I ended up recording my entire desktop instead of a given window (see #31).

Gif Recorder has a much simpler and more effective UI: its window is a frame (transparent, with borders and a stop button at the bottom). You resize this window to enclose the area to record, and click the Record button.

That was it. No compression step, no complicated instructions, no out of memory messages. It just worked.

Unfortunately, it truncated the last seconds of the gif, which is why I've been searching for an alternative.

Seth-Johnson commented 7 years ago

I can reproduce the same ffmpeg bug that @Arkni mentioned. ffmpeg continues to run even after Silentcast has finished. Also on Ubuntu /bin/sh is dash, not bash, so your install script did not work for me (you used at least one bash only feature). I suggest changing the shebang to:

/usr/bin/env bash

which is more portable (although /bin/bash should theoretically work as well).

colinkeenan commented 7 years ago

Thank you for testing right away. I am disappointed there is such a huge bug as ffmpeg not stopping.

I noticed the ffmpeg still running just when I was testing running silentcast while it was already running. I didn't see it happen before, but maybe there was some change I'm not aware of that caused this issue. I will test that now.

colinkeenan commented 7 years ago

I am not experiencing this issue of ffmpeg still running, but it's a huge issue that I will need to solve. For the moment, I changed the shebang in the install script as you suggested, @Seth-Johnson. I would prefer to write a script that always works though and will look into that.

One way to solve the ffmpeg still running issue would be to go back to using pkill -f ffmpeg, but I wanted to leave open the option of recording silentcast in action using ffmpeg directly. (This version doesn't support using silencast to record itself yet.)

What I don't understand about this killing ffmpeg bug is that there should be a dialog that pops up if my kill routine fails. And, I have verified that dialog works. It seems to mean that the kill command successfully kills something.

colinkeenan commented 7 years ago

@Seth-Johnson and @Arkni

I have released v3.01 which, after trying to kill the specific ffmpeg process that was spawned immediately goes ahead and does a pkill -f ffmpeg. Actually, for all the spawned commands, I'm doing \bin\sh -c .... I wonder if this has something to do with it.

Please let me know if it kills ffmpeg now. If not, I will remove \bin\sh -c from the command because it was working before.

colinkeenan commented 7 years ago

Oohps. I thought it was an easy fix, but it's causing segfault core dump. I didn't even test before releasing. stupid. I will work on this.

colinkeenan commented 7 years ago

Ok. It was an easy fix but I thought I didn't need to deal with errors when trying to pkill -f ffmpeg, but I did. It's good now. Please verify it stops ffmpeg now.

Also, @Arkni, in looking through the steps you listed, I noticed you didn't say anything about the file browser popping up at the end when the green rectangle returned. Did it open the file browser for you?

colinkeenan commented 7 years ago

@Arkni I just want to be sure - when you ran ./silentcast did you first cd scpkg or really run scpkg/silentcast? Because I moved the files that would be packaged, including the binary, into scpkg. If you didn't run ./silentcast from scpkg, then you were actually running an older version.

Arkni commented 7 years ago

Also, @Arkni, in looking through the steps you listed, I noticed you didn't say anything about the file browser popping up at the end when the green rectangle returned. Did it open the file browser for you?

Sorry, I forgot to mention that step. Yes, it did open the file browser.

Also, the bug about ffmpeg is fixed for me.

@Arkni I just want to be sure - when you ran ./silentcast did you first cd scpkg or really run scpkg/silentcast? Because I moved the files that would be packaged, including the binary, into scpkg. If you didn't run ./silentcast from scpkg, then you were actually running an older version.

I was running the latest version of yesterday. I compiled it and cd to scpkg to run ./silentcast.

In case you're replaying to my comment about elementary OS. Then yes you're right, I was running an older version. I did that test +5 days ago but didn't had the time to report it. I will retest this evening with the latest changes and report back.

sbrl commented 7 years ago

I've just managed to compile it and take a look! It's a huge improvement (the interface is much more streamlined for one), but there's a few things I've noticed that bother me a bit.

  1. The --version flag has disappeared
  2. You can't resize the green box
  3. If you use the F2 window to resize, then it resets the size if you drag the green box around - but doesn't update the size in the window. I'd prefer it if I could drag the edges of the box to resize the box - a bit like gimp's crop tool.
  4. When moving the green box, if I move it to the bottom of the screen, it hides the help text. Perhaps the help text could move around such that it's always on the screen?
  5. When finished recording, it says that I can now manipulate the pngs - but if I didn't understand silentcast already, I wouldn't know where it put them. Perhaps a flag or GUI option to control this would work well?
colinkeenan commented 7 years ago

@sbrl Thank you for your feedback. I've got some questions for you and agree that some issues need to be solved.

  1. By default Gtk+ treats command-line arguments as files. However, if you F1->About Silentcast, you will see the version. For now, since there's no files to be opened, I've disabled the default behaviour. In the future, I will add command-line options to duplicate the command-line abilities of the previous versions of Silentcast.
  2. The very first thing I got working many months ago was the ability to easily resize the green box in many ways. Is right-click and scroll-wheel not working for you? Or, do I need to make the mouse controls more obvious?
  3. See my question 2.
  4. Yes, I agree. When I start improving the UI again, that's the first thing I will fix.
  5. You are suggesting I should open the file browser at this point. I agree something needs to be done, but I was having issues with opening the file browser before the end. By the way, did it open the file browser at the end?

Please answer my question about whether right-click and scroll wheel are working or I need to make those controls more obvious. Thanks again for providing feedback.

sbrl commented 7 years ago

@colinkeenan

  1. Ah right
  2. Ah right - upon checking they do indeed work. Yeah, making that a bit more obvious would be welcome. Also, could you add Ctrl + Left click and drag an alternative here? I'm using a laptop touchpad, and right click + drag is just awkward with the style of right click I use (2 finger tap)
  3. Right.
  4. Cool :D
  5. Opening the file browser would work, but you could always just display the file path on screen. And yeah, it did infact open the file browser correctly at the end. And I've got a tmpfs mounted as my /tmp directory, so I'd prefer it if I could tell silent cast to send images there, since then they won't hit the disk.
colinkeenan commented 7 years ago

Looking at my code, I realized it was pretty easy to move the text around on the screen and so fixed that, but just by hardcoding the number of pixels needed on each edge. If that isn't working for some people, I will eventually figure out how to calculate the number of pixels needed. I just chose round numbers like 100, 200, and 300, which worked reasonably well on my screen.

I have also included your idea of displaying the directory since that's also very easy.

I will take a look at what I have to do to get CTRL+Left Click to work. I kind of remember trying something like that before and couldn't get it to work. If I remember right, I could get Gtk+ to detect CTRL+key but not CTRL+mouse-button.

If I can quickly get CTRL+Right-click to work, I'll include that in a 3.02 release soon. Otherwise, I'll release 3.02 without that because I think these other fixes are important.

colinkeenan commented 7 years ago

By the way, you can set the working directory from F1->Configuration which has the same options as the original Silentcast.

colinkeenan commented 7 years ago

I pushed the fixes so far so you can test even though not released as 3.02 yet. The about dialog will show 3.02 though.

colinkeenan commented 7 years ago

I went ahead and released 3.02 that makes all the fixes you suggested except CTRL+Left-Click because that one will be difficult. The silentcast directory shows up in the edit pngs dialog and is selectable.

colinkeenan commented 7 years ago

@sbrl Please let me know how this release works for you. Thanks.

colinkeenan commented 7 years ago

@Seth-Johnson @Arkni @sbrl

Last night I finally ran valgrind on silentcast again and discovered a mistake. My one and only use of malloc was off by 1 because I didn't realize strlen didn't include the terminating nul character. I fixed that (at first by adding 1 and then by using g_strdup instead since it does full error checking automatically)

However, there are many read/write errors coming from cairo that only happen when dialogs are displayed. I have filed a bug report against cairo because valgrind doesn't show any silentcast functions causing the error.

Here is the bug report: https://bugs.freedesktop.org/show_bug.cgi?id=100713

This is against cairo 1.14.8.

I would like to know if the same errors are happening on other versions of cairo.

Please run:

valgrind -v --tool=memcheck --leak-check=full --leak-resolution=high --num-callers=20 --log-file=vgdump scpkg/silentcast

and search on "Invalid"

Arkni commented 7 years ago

I got 3 Invalid but I guess they are not related to cairo:

[...]
==32730== 
--32730-- memcheck GC: 17216 nodes, 1856 survivors (10.8%)
==32730== Invalid write of size 1
==32730==    at 0x4C2DAE3: strcpy (vg_replace_strmem.c:506)
==32730==    by 0x40E504: get_array_of_pngs (SC_temptoanim.c:129)
==32730==    by 0x40E5FF: delete_pngs (SC_temptoanim.c:149)
==32730==    by 0x40FD09: call_nextfunc (SC_temptoanim.c:364)
[...]
==310== 40 errors in context 8 of 527:
==310== Invalid write of size 1
==310==    at 0x4C2DAE3: strcpy (vg_replace_strmem.c:506)
==310==    by 0x40E504: get_array_of_pngs (SC_temptoanim.c:129)
==310==    by 0x40E5FF: delete_pngs (SC_temptoanim.c:149)
==310==    by 0x40FD09: call_nextfunc (SC_temptoanim.c:364)
==310==    by 0x410317: SC_spawn (SC_temptoanim.c:455)
[...]
==32730== 40 errors in context 8 of 416:
==32730== Invalid write of size 1
==32730==    at 0x4C2DAE3: strcpy (vg_replace_strmem.c:506)
==32730==    by 0x40E504: get_array_of_pngs (SC_temptoanim.c:129)
==32730==    by 0x40E5FF: delete_pngs (SC_temptoanim.c:149)
==32730==    by 0x40FD09: call_nextfunc (SC_temptoanim.c:364)
==32730==    by 0x410317: SC_spawn (SC_temptoanim.c:455)
[...]

To see the full dump, head to: https://gist.github.com/Arkni/f0a2968691e91f0a78f82446ad1095bb

By the way, this is against cairo 1.13.0 on Ubuntu 14.04.5 LTS

colinkeenan commented 7 years ago

@Arkni,

Thanks, but that is the error I already fixed. Are you sure that you have the latest? I originally was using malloc strlen but when I saw that error, changed it to malloc strlen + 1 which fixed the problem. And then later, realized there's a better way that does error checking, which is strdup or even better, g_strdup.

Please take a look at SC_temptoanim.c line 129. If you see a malloc nearby, you have an old version.

It's good to know that the problem I'm seeing is probably with cairo 1.14.8 as I reported in the bug.

Arkni commented 7 years ago

Oops, my bad! I think I need my dose of cafe. Sorry :/

I fetched the latest changes and compiled then analysed the binary using the command you mentioned in your previous comment. I got no Invalid in the new dump. So everything is ok for me.

In case you want to see the new dump, please head to: https://gist.github.com/Arkni/7134074b1a29e588c78a25fe63c5c241

sbrl commented 7 years ago

Heya!

Just cloned + tested the latest commit. Works a lot better! The display of the directory that contains the raw images is helpful, and the moving the text to the top / bottom depending on the green box is useful too - though if I've got a selection selected that covers most of the screen, then the text is still hidden. Perhaps it could be shown inside the box if there's no space at either the top or bottom?

I thought of another potential improvement: Include a configuration option to optionally hide the mouse pointer. Perhaps I should file a separate bug about that though?

Great work so far! :star2:

colinkeenan commented 7 years ago

@sbri Thanks.

I'm confused about why you would hide the mouse pointer though. Why? And, more importantly, how would you stop recording if you couldn't even see where to click the icon?

Yes - go ahead and open another issue for the trackpad problem and any other ideas you have (but explain why and how you would use a new feature like hiding the mouse pointer.) When I have time, I'll work on those. Thank you again.

sbrl commented 7 years ago

Ah, I didn't mean 'hide the mouse pointer' on the screen - only in the recording. Is that possible? Sometimes I'd like to record something that's happening on the screen without the mouse pointer, but I don't have anywhere to put it that's out of the way. Perhaps an alternative here would be a keyboard shortcut to stop the recording?

colinkeenan commented 7 years ago

Thanks for opening the trackpad issue. You can open this mouse pointer issue too, but I don't see an easy way to do it. I have not heard of ffmpeg having an option to ignore the mouse pointer when grabbing the screen. Silentcast is minimized when recording and minimized Gtk+ applications ignore events while minimized (which makes my code a little easier). There's probably a way to get a Gtk+ application to continue listening for keyboard or mouse events while minimized. There may also be another application that hides the mouse pointer until a certain keyboard or mouse event. So, go ahead and open the issue. If you find an application that does the job, comment on that too. Thanks.

sbrl commented 7 years ago

@colinkeenan You're welcome! The cursor issue isn't a priority at all, really - just an idea of mine I had.