[WIP] Snapshot record video #11744

Open
wants to merge 39 commits into
from

Conversation

Projects
None yet
9 participants
@Jules2010

Jules2010 commented Jan 31, 2018

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

This change add Snapshot to the ability to record videos of iOS applications, using the same method used to take screenshots.

Description

Added ability to record of video within the simulator

laullon added some commits Aug 10, 2017

Replace a "file watcher" for a "simple http server" for the "Snapshot…
…Helper.swift" -> "snapshot.rb" communication to remove a dependency.

Feedback addressed @KrauseFx
Back to work after '[snapshot] Add support for concurrent simulators …
…with Xcode 9 (#9570)'  change

(Only for xCode 8 for now)
Merge branch 'master' into snapshot-record-video
* master: (96 commits)
  [snapshot] Fix unused result warning in helper (#10662)
  Typo Fix (#10661)
  Version bump to 2.62.1 (#10670)
  Support manual signingStyle (#10508)
  Dont count lane switches in metrics (#10659)
  Add missing html tag (#10657)
  Deprecates add_id_info_limits_tracking in favor of add_id_info_uses_idfa (#10653)
  [fastlane core] fix unit tests when Xcode 9.0 not installed (#10655)
  Add automatic commiting of mkdocs.yml for docs generation (#10578)
  [Client] Handle Faraday::ParsingError as part of Client#with_retry logic (#10646)
  Revert "[BuildWatcher] Handle parse error while waiting for build processing (#10621)" (#10647)
  [BuildWatcher] Handle parse error while waiting for build processing (#10621)
  Remove 'zip' example from update_info_plist (#10494)
  Make plugin details optional (#10628)
  fix leaking sw_vers (#10630)
  Update snapshot instructions (#10626)
  Update update instructions of snapshot (#10627)
  Don't add unneeded new-line when asking for user input (#10629)
  Forward match keychain password into cert (#10619)
  SW_VERS should be lowercase (#10625)
  ...

# Conflicts:
#	README.md
#	cert/README.md
#	snapshot/README.md
#	snapshot/lib/assets/SnapshotHelper.swift
@googlebot

This comment has been minimized.

Show comment Hide comment
@googlebot

googlebot Jan 31, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added the cla: no label Jan 31, 2018

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Jan 31, 2018

@laullon please will you sign the CLA again here please. I'm going to try and get this pushed through without the tcp listener.

@laullon please will you sign the CLA again here please. I'm going to try and get this pushed through without the tcp listener.

Jules2010 added some commits Jan 31, 2018

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Jan 31, 2018

Cheers.

I’ve not done any ruby yet.

Initially just get something working, I have 6 languages and 8 devices to produce videos for, I need something to automate this task.

However I may need to add some extra features, not sure what au be needed yet.

Cheers.

I’ve not done any ruby yet.

Initially just get something working, I have 6 languages and 8 devices to produce videos for, I need something to automate this task.

However I may need to add some extra features, not sure what au be needed yet.

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Jan 31, 2018

I timed my existing snapshot lane last night and found I'm going to need to remove some the functionality to make it fit within 30 seconds.

I'm hoping to do all this in-app, without having to pause the video.

I've already got a start and finish slide, with fading, that going to be around 7 seconds.

I figure I can dismiss view controllers without animation, then push without animation, to speed up the transition between several screens.

At least that's my plan.

Otherwise I may need to add some kind of pause then fade to fastlane.

I timed my existing snapshot lane last night and found I'm going to need to remove some the functionality to make it fit within 30 seconds.

I'm hoping to do all this in-app, without having to pause the video.

I've already got a start and finish slide, with fading, that going to be around 7 seconds.

I figure I can dismiss view controllers without animation, then push without animation, to speed up the transition between several screens.

At least that's my plan.

Otherwise I may need to add some kind of pause then fade to fastlane.

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Jan 31, 2018

I'm struggling, can't figure out how to call the start / stop methods :( c6458f0

I'm struggling, can't figure out how to call the start / stop methods :( c6458f0

@laullon

This comment has been minimized.

Show comment Hide comment
@laullon

laullon Feb 1, 2018

call the start / stop methods from where? UITest?

laullon commented Feb 1, 2018

call the start / stop methods from where? UITest?

@laullon

This comment has been minimized.

Show comment Hide comment
@laullon

laullon Feb 1, 2018

I signed it!

laullon commented Feb 1, 2018

I signed it!

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Feb 1, 2018

Thanks.

Yes from the UITest any help appreciated.

Thanks.

Yes from the UITest any help appreciated.

@laullon

This comment has been minimized.

Show comment Hide comment
@laullon

laullon Feb 1, 2018

sure... That was the purpose of the "tcp listener"/"web server" ;)

laullon commented Feb 1, 2018

sure... That was the purpose of the "tcp listener"/"web server" ;)

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Feb 1, 2018

Yeah. I’m just keen to get it merged, then hopefully it will get more features and discussion.

Any ideas how I need to add the start / stop in to the swift files, I couldn’t see it on snapshot or simulator.

Maybe it needs exposing somehow?

I’m a bit out of my depth with ruby.

Yeah. I’m just keen to get it merged, then hopefully it will get more features and discussion.

Any ideas how I need to add the start / stop in to the swift files, I couldn’t see it on snapshot or simulator.

Maybe it needs exposing somehow?

I’m a bit out of my depth with ruby.

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Feb 1, 2018

I signed it!

I signed it!

@KrauseFx KrauseFx changed the title from Snapshot record video to [WIP] Snapshot record video Feb 1, 2018

@KrauseFx

Hey, cool to see some movement in this again. Please make sure to avoid adding a web server to this, as it adds a lot of complexity to the code base. I'm using Request Change to make sure this doesn't get merged until it's ready, it's still WIP, right?

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Feb 1, 2018

@KrauseFx I’ve taken the web server stuff out.

However I’m not familiar with ruby and am struggling to add calls to the start and stop functions to the swift can you help with this small change please?

@KrauseFx I’ve taken the web server stuff out.

However I’m not familiar with ruby and am struggling to add calls to the start and stop functions to the swift can you help with this small change please?

@laullon

This comment has been minimized.

Show comment Hide comment
@laullon

laullon Feb 1, 2018

@KrauseFx any recommendation on how to communicated the UITest with the Fastlane process to start/stop the recording ?

I did used the web server because it was very easy of implement, I also tested to use a flag file, but that was not ok because I did used a 3rd party library to watch for file system changes.

laullon commented Feb 1, 2018

@KrauseFx any recommendation on how to communicated the UITest with the Fastlane process to start/stop the recording ?

I did used the web server because it was very easy of implement, I also tested to use a flag file, but that was not ok because I did used a 3rd party library to watch for file system changes.

@laullon laullon referenced this pull request Feb 5, 2018

Closed

[snapshot] Ability to record video #10121

4 of 4 tasks complete
@getaaron

This comment has been minimized.

Show comment Hide comment
@getaaron

getaaron Feb 11, 2018

Contributor

There are a few other options:

I think the STDOUT approach (using print() and IO) is least likely to require additional dependencies, but it may not be the most elegant.

@KrauseFx any thoughts here?

Contributor

getaaron commented Feb 11, 2018

There are a few other options:

I think the STDOUT approach (using print() and IO) is least likely to require additional dependencies, but it may not be the most elegant.

@KrauseFx any thoughts here?

-# remove the '#' to clear all previously generated screenshots before creating new ones
-# clear_previous_screenshots true
+
+# Where should the resulting videos be stored?

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

this comment should take into account that by default no video will be create. So probably something like:

If you configure snapshot to generate of your tets, where should it be stored?

@janpio

janpio Feb 16, 2018

Collaborator

this comment should take into account that by default no video will be create. So probably something like:

If you configure snapshot to generate of your tets, where should it be stored?

+
+# clear_previous_screenshots true # remove the '#' to clear all previously generated screenshots before creating new ones
+
+# Choose which project/workspace to use

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

unrelated new feature or just new in SnapfileTemplate?

@janpio

janpio Feb 16, 2018

Collaborator

unrelated new feature or just new in SnapfileTemplate?

+
+require 'open3'
+
+module Snapshot

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

FYI: When you rebase onto current master, this module will alredy exist in snapshot/module relative from here. (Just so you are aware and nobody is confused when reviewing - as I was)

@janpio

janpio Feb 16, 2018

Collaborator

FYI: When you rebase onto current master, this module will alredy exist in snapshot/module relative from here. (Just so you are aware and nobody is confused when reviewing - as I was)

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

If not, and this is just a useless copy, one would have to make sure if there are no important changes here.

@janpio

janpio Feb 16, 2018

Collaborator

If not, and this is just a useless copy, one would have to make sure if there are no important changes here.

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

Quick comparison shows that they are indeed identical. This is probably just an artifact (that has to be fixed before a merge).

@janpio

janpio Feb 16, 2018

Collaborator

Quick comparison shows that they are indeed identical. This is probably just an artifact (that has to be fixed before a merge).

@@ -7,6 +7,7 @@ module Snapshot
class Options
def self.available_options
output_directory = (File.directory?("fastlane") ? "fastlane/screenshots" : "screenshots")
+ video_output_directory = (File.directory?("fastlane") ? "fastlane/videos" : "videos")

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

does this folder already exist by default?

@janpio

janpio Feb 16, 2018

Collaborator

does this folder already exist by default?

@@ -134,6 +137,39 @@ def execute(retries = 0, command: nil, language: nil, locale: nil, launch_args:
end)
end
+ def start_recording(dir_name, device, name)

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

maybe add video to the method name to make it clearer what this is?

@janpio

janpio Feb 16, 2018

Collaborator

maybe add video to the method name to make it clearer what this is?

+ def start_recording(dir_name, device, name)
+ device_udid = TestCommandGenerator.device_udid(device)
+ pid = @recording_pid[device_udid]
+ return unless pid.nil?

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

what exactly does this do and why?

@janpio

janpio Feb 16, 2018

Collaborator

what exactly does this do and why?

@@ -134,6 +137,39 @@ def execute(retries = 0, command: nil, language: nil, locale: nil, launch_args:
end)
end
+ def start_recording(dir_name, device, name)
+ device_udid = TestCommandGenerator.device_udid(device)

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

extract into well named method as it is repeated below

@janpio

janpio Feb 16, 2018

Collaborator

extract into well named method as it is repeated below

+ def stop_recording(device)
+ device_udid = TestCommandGenerator.device_udid(device)
+ pid = @recording_pid[device_udid]
+ return if pid.nil?

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

what exactly does this do and why?

@janpio

janpio Feb 16, 2018

Collaborator

what exactly does this do and why?

+ end
+ end
+
+ def stop_recording(device)

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

also add video to method name maybe?

@janpio

janpio Feb 16, 2018

Collaborator

also add video to method name maybe?

@@ -66,6 +67,8 @@ def take_screenshots_simultaneously
device_batches = launcher_config.devices.map { |d| [d] }
end
+ @recording_pid = {}

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

should this also be in the _xcode_8 version of this file?

@janpio

janpio Feb 16, 2018

Collaborator

should this also be in the _xcode_8 version of this file?

+ stop_recording
+ end
+
+ def start_recording(dir_name, device_type, name)

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

if this is identical to above, can this be moved outside of these files somehow and unified?

@janpio

janpio Feb 16, 2018

Collaborator

if this is identical to above, can this be moved outside of these files somehow and unified?

@@ -89,6 +89,7 @@ def execute(command: nil, language: nil, locale: nil, device_type: nil, launch_a
loading: "Loading...",
error: proc do |output, return_code|
ErrorHandler.handle_test_error(output, return_code)
+ stop_recording

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 16, 2018

Collaborator

should this be in the other, non _xcode_8 version of this file?

@janpio

janpio Feb 16, 2018

Collaborator

should this be in the other, non _xcode_8 version of this file?

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Feb 17, 2018

Hi @janpio

I'd just like to mention that this pull request as it stands doesn't provide any additional functionality as it stands. It's based on PR #10121 I'm not 100% sure about your questions.

However this pull request has the http listener code removed and therefore doesn't have any means of triggering the video recording.

I started this PR at that point, in the hope that someone would step in and advise and contribute to it's completion. I understand that @laullon has moved on from his PR with other projects and doesn't have time to contribute further.

For information purposes, I have created a new branch https://github.com/Jules2010/fastlane/tree/snapshot-record-video-with-audio which includes @laullon initial work and QuickTime Player to also record sound.

I hope this can move forward.

Jules.

Hi @janpio

I'd just like to mention that this pull request as it stands doesn't provide any additional functionality as it stands. It's based on PR #10121 I'm not 100% sure about your questions.

However this pull request has the http listener code removed and therefore doesn't have any means of triggering the video recording.

I started this PR at that point, in the hope that someone would step in and advise and contribute to it's completion. I understand that @laullon has moved on from his PR with other projects and doesn't have time to contribute further.

For information purposes, I have created a new branch https://github.com/Jules2010/fastlane/tree/snapshot-record-video-with-audio which includes @laullon initial work and QuickTime Player to also record sound.

I hope this can move forward.

Jules.

@janpio

This comment has been minimized.

Show comment Hide comment
@janpio

janpio Feb 17, 2018

Collaborator

Yes, I got that, saw the other PR as well.

I tried to understand what is already here and see if this is in an optimal state to solve the remaining problem - because I really like the possible functionality here! Using snapshot to record videos of the app would really be an awesome feature.

Maybe @laullon can also take a look at my comments?

Collaborator

janpio commented Feb 17, 2018

Yes, I got that, saw the other PR as well.

I tried to understand what is already here and see if this is in an optimal state to solve the remaining problem - because I really like the possible functionality here! Using snapshot to record videos of the app would really be an awesome feature.

Maybe @laullon can also take a look at my comments?

@getaaron

This comment has been minimized.

Show comment Hide comment
@getaaron

getaaron Mar 27, 2018

Contributor

@Jules2010 How is this work coming along? Were you able to get it working for your project locally without the HTTP server?

Contributor

getaaron commented Mar 27, 2018

@Jules2010 How is this work coming along? Were you able to get it working for your project locally without the HTTP server?

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Mar 27, 2018

@getaaron unfortunately not, I didn’t write the code originally. But I was able to add sound via QuickTime and use ffmpeg to join the audio and video together.

However my code isn’t pretty and has hardcoded paths.

I did mention to @laullon on twitter but he obviously didn’t find the time to help further.

@getaaron unfortunately not, I didn’t write the code originally. But I was able to add sound via QuickTime and use ffmpeg to join the audio and video together.

However my code isn’t pretty and has hardcoded paths.

I did mention to @laullon on twitter but he obviously didn’t find the time to help further.

@guda1

This comment has been minimized.

Show comment Hide comment
@guda1

guda1 Apr 13, 2018

done

@Lausbert

This comment has been minimized.

Show comment Hide comment
@Lausbert

Lausbert Apr 30, 2018

If someone is looking for inspiration for a pull request without a http server, checkout my proof of concept: https://github.com/Lausbert/Snaptake

Lausbert commented Apr 30, 2018

If someone is looking for inspiration for a pull request without a http server, checkout my proof of concept: https://github.com/Lausbert/Snaptake

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 Apr 30, 2018

@Lausbert does this add sound to the videos too? My hack uses QuickTime and merged them together.

@Lausbert does this add sound to the videos too? My hack uses QuickTime and merged them together.

@getaaron

This comment has been minimized.

Show comment Hide comment
@getaaron

getaaron Apr 30, 2018

Contributor

@Lausbert I really like this approach; what do you think about setting this up as a fastlane plugin? We could add a comment about it in the Snapfile and link to it in the docs.

Contributor

getaaron commented Apr 30, 2018

@Lausbert I really like this approach; what do you think about setting this up as a fastlane plugin? We could add a comment about it in the Snapfile and link to it in the docs.

@Lausbert

This comment has been minimized.

Show comment Hide comment
@Lausbert

Lausbert May 1, 2018

@Jules2010 I haven't tested it so far, since all I needed to do in my specific case was to add a jingle later on with ffmpeg.

@getaaron Sounds great, but what about the changes in snapshotHelper.swift? Did I miss anything or is it impossible to integrate them by adding a plugin?

Lausbert commented May 1, 2018

@Jules2010 I haven't tested it so far, since all I needed to do in my specific case was to add a jingle later on with ffmpeg.

@getaaron Sounds great, but what about the changes in snapshotHelper.swift? Did I miss anything or is it impossible to integrate them by adding a plugin?

@Jules2010

This comment has been minimized.

Show comment Hide comment
@Jules2010

Jules2010 May 1, 2018

@Lausbert the simulator currently doesn’t provide sound. I filed a radar and was told it was a feature request.

@Lausbert the simulator currently doesn’t provide sound. I filed a radar and was told it was a feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment