Skip to content
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.

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

asciicast v2 #35

Merged
merged 3 commits into from
Nov 10, 2020
Merged

asciicast v2 #35

merged 3 commits into from
Nov 10, 2020

Conversation

hoehermann
Copy link
Contributor

Maybe you want this for #23.

Copy link
Owner

@dhobsd dhobsd left a comment

Choose a reason for hiding this comment

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

Hey, thanks so much for this contribution! Sorry for the delay in looking at it; I've been neglecting my GitHub repos a bit.

I'd like to merge this, but there are a couple things that I was hoping we could fix up before doing so.

  • The delayed audio feature compensation I think is super useful! I would like to merge it in its own PR, though. Could you move that commit to its own PR?
  • One comment about putting the duration in the header.
  • I wonder whether you could make this optionally output the v2 format. In particular, the player that ships in this repo can't play the v2 format. I'd like for the tool to be able to output v1 by default until the player also supports v2.

Are you able to address those points?

Thanks again for the PR!

src/output.c Outdated
@@ -271,8 +271,9 @@ outputproc(struct outargs *oa)
}

end:
/* this is breaking the format, but still useful information */
fprintf(evout, "{\"duration\":%0.3f}", dur / 1000);
if (oa->append_trailer) {
Copy link
Owner

Choose a reason for hiding this comment

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

Per the v2 format, duration is a thing that is allowed in the header. If we're going to print it out anywhere, I'd like for it to be there rather than as an optional thing that is outside the spec.

@hoehermann
Copy link
Contributor Author

hoehermann commented Apr 13, 2020

I can do that. Don't know when, though.

I need to cut parts of the cast including the audio. For this reason, I also worked on an editor, but it is not ready yet. Here is an example: https://ssl.hehoe.de/zeug/asciinema-editor/

@dhobsd
Copy link
Owner

dhobsd commented Apr 16, 2020

Very cool! Thank you!

@hoehermann hoehermann force-pushed the asciicast-v2 branch 2 times, most recently from 81c9b58 to 9191736 Compare July 12, 2020 17:47
@hoehermann
Copy link
Contributor Author

hoehermann commented Jul 12, 2020

I finally got around to toy with this again. As you requested, asciicast v2 format is now optional. Duration is stored in the header without breaking the specification.

I removed all "improvements" relating to audio. On my system, somehow related to pulseaudio, there is a significant delay between the call to audio_start and the time of the audio samples being recorded. Foolish me assumed this delay would be static. Now I observed random delays between 0 and more than a second. I could probably work around this by mimicking the mechanics used to mute audio. This would mean calling audio_start only once. If the user wants to pause audio, audio_stop would not be called, but samples should be discarded. I failed to implement this quickly, so I resort to doing my recordings in one go. At least for now.

@dhobsd
Copy link
Owner

dhobsd commented Oct 22, 2020

Hey there! I'm terribly sorry for the delay again -- personal stuff has resulted in being a poor OSS steward.

I've made some changes that introduce a conflict here, but I'd still like to merge this. I'm intending to do so tomorrow morning, or next Thursday if that doesn't work out.

Are you still having issues with delays related to Pulse? I'm interested in what you've learned since then, if you wouldn't mind filing an issue in the repo.

Finally, I'd like to circle back to editing tools. Did you end up doing more work here? Licencing around the code linked is unclear, so I'm hesitant to base any work off of it without permission.

@hoehermann
Copy link
Contributor Author

hoehermann commented Oct 22, 2020

Hey there! I'm terribly sorry for the delay again -- personal stuff has resulted in being a poor OSS steward.

No worries – we are all doing this for fun, are we not? Obligations and pressure are no fun. :)

Are you still having issues with delays related to Pulse?

Yes, but it is not castty's fault at all. I still have not found the root of the problem. It seems to be related to the cheap USB capture device I am using. I am also experiencing problems when recording with ffmpeg. There is a certain random delay between opening the device and when the samples are actually being recorded. I can work around it by starting pavucontrol and leave it open. Apparently pavucontrol is continuously polling the hardware for the most recent samples, keeping it busy. That works for ffmpeg at least, I have not tried with castty since I discovered this behaviour.

Finally, I'd like to circle back to editing tools. Did you end up doing more work here?

Unfortunately, I have not worked towards editing, but I did use it productively and, based on user reports, improved the playback functionality. Seeking looks okay now in most cases. Sometimes the HTML terminal emulator does not clear the screen properly, but I have not figured out how to compile the most recent version. :/

Licencing around the code linked is unclear, so I'm hesitant to base any work off of it without permission.

Oh, dang. That is me being a poor OSS steward. I just GPL'ed casttycut. :)

@dhobsd
Copy link
Owner

dhobsd commented Nov 10, 2020

Thanks so much for this work. I wish there was a cleaner way to insert the header, because it feels a bit brittle. I think eventually castty should do an atexit(3)-ish thing where it writes a header to a file and then merges the events into that file. I'll make an issue to fix that (who knows when that'll get gotten around to, hah). But in the meantime, thanks again for the contributions!

@dhobsd dhobsd closed this Nov 10, 2020
@dhobsd dhobsd reopened this Nov 10, 2020
@dhobsd dhobsd merged commit 333a2ba into dhobsd:master Nov 10, 2020
@hoehermann hoehermann deleted the asciicast-v2 branch January 2, 2021 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants