Skip to content

Conversation

mmulet
Copy link
Contributor

@mmulet mmulet commented Jun 27, 2025

Fixes #337

  • save the modified video (trimming and cropping), so Gifski could also be used to cropping video. I'm thinking a "Export as Video…" in the "File" menu.
  • Export original audio

I implemented export of the original video, let me know what you think of the UI. Meanwhile, I will work on inserting the original audio track into the export.

@mmulet
Copy link
Contributor Author

mmulet commented Jun 27, 2025

  • Added sound export support (works with trimming and speed changes)
  • Fix: a bug where the video preview would flash black when focus changed between export windows.
  • Fix: Can have multiple export windows open at the same time now.
  • Fix: Now, it only deleted the temp video when the window closes, not on save (wasn't really a bug, I just don't know why I did it that way).

Bug: I found a bug where if you change the speed of the video, TrimmingAVPlayer.timeRangeDidChange will be called with the full range of the video instead of the trimmed range.
So, if the time range is not full, and you change the speed, then the export time range will export the entire video instead of trimmed range.
I don't think this a bug from this pull request, it seems to be a bug in TrimmingAVPlayer itself. Is there a reason why it should be like this?

Nevermind, I went to take a video of this bug, and now I can't reproduce it.

@sindresorhus
Copy link
Owner

The video export is unlikely to take a long time, since most people use it for GIFs. So I think we can just show a sheet with a system spinner and cancel button if the video is longer than 20 seconds. And then afterwards, just immediately show the save dialog. One less click. And we don't really need the preview as the user can already preview in the main Gifski UI.

@sindresorhus
Copy link
Owner

You also need to add MP4 to NSExportableTypes in Info.plist.

@mmulet
Copy link
Contributor Author

mmulet commented Jul 7, 2025

  • The video export is unlikely to take a long time, since most people use it for GIFs. So I think we can just show a sheet with a system spinner and cancel button if the video is longer than 20 seconds. And then afterwards, just immediately show the save dialog. One less click. And we don't really need the preview as the user can already preview in the main Gifski UI.
  • Add MP4 to NSExportableTypes in Info.plist.
  • Command+E shortcut to export modified video.
  • .mov -> .mp4
  • restore canBeginTrimming comment in Utilities.swift
  • Sorry, this was a mistake, shouldn't have been in the commit.
  • translatedBy(point: -> translated(by:
  • taskGroupMap -> concurrentMap
  • ClosedRange where Bound == Double -> ClosedRange<Double>

@sindresorhus
Copy link
Owner

The audio preservation is complicating this a lot, when it's not directly needed for Gifski. Maybe we should just drop it? We could show an alert once, the first time the user tries to export a video with audio tracks.

@sindresorhus
Copy link
Owner

Also try to clean up and simplify the code more.

@mmulet
Copy link
Contributor Author

mmulet commented Jul 8, 2025

The audio preservation is complicating this a lot, when it's not directly needed for Gifski. Maybe we should just drop it? We could show an alert once, the first time the user tries to export a video with audio tracks.

I agree.

Also try to clean up and simplify the code more.

Yeah, I'll take out the audio and simplify the PR a bunch.

@mmulet mmulet requested a review from sindresorhus July 8, 2025 19:56
@mmulet
Copy link
Contributor Author

mmulet commented Jul 8, 2025

  • Remove audio
  • Remove unused code
  • Simplify code
  • Simplified it quite a bit.
  • Show error with Alert if the export fails instead of in the sheet.
  • Check error for really long text
  • Moved to appState.error
  • Drop cancellation
  • Move the fileExporter modifier out of the sheet
  • Remove unnecessary FileManager.removeItem
  • The setter is empty, meaning when the file export sheet is dismissed (e.g., user cancels), no state is updated.

@mmulet
Copy link
Contributor Author

mmulet commented Jul 8, 2025

  • warning on export if audio track is present.

@mmulet
Copy link
Contributor Author

mmulet commented Jul 9, 2025

  • Fix: If another alert (like bounce warning) occurs when you activate Save Modified Video, the filexporter won't show and the state will be stuck on .exported. This fixes that by reassigning the state to force a swiftUI draw and bring up the file exporter.

@mmulet
Copy link
Contributor Author

mmulet commented Jul 21, 2025

  • ExportModifiedVideoView.State -> ExportModifiedVideoState
  • GifGenerator.trackSize -> GifGenerator.trackDimensions
  • Style fixes like \s*/ -> */, else { -> else \n {
  • AVAsset.VideoMetadata.originalVideoHasAudio -> AVAsset.VideoMetadata.hasAudio
  • Export video Limitation -> Export Video Limitation
  • ExportModifiedVideoState.isExporting getters for isProgressSheetPresented, etc
  • Use AI to check for typos, bugs, and potential improvements for all the code in this pull request.
  • exported(..) -> finished(...)
  • GifGenerator.dimensionsAsSize -> GifGenerator.dimensionsAsCGSize

@sindresorhus
Copy link
Owner

#339 (comment) is not done.

@mmulet
Copy link
Contributor Author

mmulet commented Aug 10, 2025 via email

@mmulet
Copy link
Contributor Author

mmulet commented Aug 11, 2025

  • Now it only shows the progress when the video is over 20 seconds long.

What else am I missing? It seems to work to me.

@sindresorhus
Copy link
Owner

That's not what the link points to. It's:

This is an unrelated independent state and should not be here.

@mmulet
Copy link
Contributor Author

mmulet commented Aug 12, 2025

That's not what the link points to. It's:

This is an unrelated independent state and should not be here.

Ah, I see now. I went and moved the ExportVideoState to its own file.

@sindresorhus
Copy link
Owner

No, that's not it. The comment is on the case audioWarning line. And I meant that that case is not related to the state and should be a independent boolean state in the view itself, not part of this enum.

@mmulet
Copy link
Contributor Author

mmulet commented Aug 12, 2025

No, that's not it. The comment is on the case audioWarning line. And I meant that that case is not related to the state and should be a independent boolean state in the view itself, not part of this enum.

Oh, that makes a lot more sense. Sorry about the confusion. I will fix it now.

@mmulet
Copy link
Contributor Author

mmulet commented Aug 12, 2025

  • Move audioWarning out of ExportModifiedVideoState

@sindresorhus
Copy link
Owner

If I trim a video, and then change the speed, the saved video does not seem to respect the trim.

The speed also seems to be additive, so if I drag it to faster multiple times, it just gets faster.

@mmulet
Copy link
Contributor Author

mmulet commented Aug 19, 2025

  • Fix: If I trim a video, and then change the speed, the saved video does not seem to respect the trim.
  • This was a bug in AVTrimmingPlayerView, not from this pr, but I fixed it anyways.
  • Remove leftovers exportModifiedVideoState.swift file
  • GifGenerator return value fix
  • .toTimeInterval > 20 -> _ > .secondons(20)
  • remove leftover case
  • Use preferredTransform in ExportModifiedVideo to correctly handle videos with metadata that they are rotated.

Support for preferredTransform

preferredTransform turned out to be a can of worms, because almost nothing else worked with it! I had to make changes to preview, crop, intents, and normal gif export to support it.

Couldn't reproduce

  • Fix: The speed also seems to be additive, so if I drag it to faster multiple times, it just gets faster.
  • I could not reproduce this bug. I've attached a video of me trying to change the speed a lot and each time it exports correctly?

@mmulet
Copy link
Contributor Author

mmulet commented Aug 19, 2025

exports just fine:

screen1.mp4

and I don't see the problem when scrubbing the speed while playing (although it does look like sometimes it does not change the play speed, which is a separate issue that I should look into*):

screen2.mp4

* I did look into it a bit. It's hard to diagnose because

  1. It doesn't always happen. It works most of the time, and I'm still having trouble triggering it reliably.
  2. When it does happen, neither setSpeed or or the on receive get an update.
.onReceive(Defaults.publisher(.outputSpeed, options: []).removeDuplicates().debounce(for: .seconds(0.4), scheduler: DispatchQueue.main)) { _ in
			Task {
				await setSpeed()
			}
		}

I'm out of time today to figure out what the problem is.

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.

Add ability to save modified original video
2 participants