Track recordings #180
Track recordings #180
Conversation
lib/ex_webrtc/recorder.ex
Outdated
| # XXX need? | ||
| try do | ||
| GenServer.call(recorder, {:add_tracks, tracks}) | ||
| catch | ||
| _exit_or_error, _e -> | ||
| Logger.error("Recorder is down, not adding tracks") | ||
| :error | ||
| end |
There was a problem hiding this comment.
Up for discussion whether this try block should go here
I don't think this is the correct place for it, I'd put it in Broadcaster logic instead
There was a problem hiding this comment.
I had the same problem some time ago - link 🙄
IMO, we can remove try catch for now. I don't know how someone could handle the error as it is related to this specific track, not the process of recording as a whole.
Recorder is also assumed to be alive all the time so if it is not, then something bad happened and it might be better for the caller to just crash.
And we can always go back to try catch if we find it useful in the future.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #180 +/- ##
==========================================
- Coverage 87.81% 84.64% -3.17%
==========================================
Files 47 49 +2
Lines 2371 2469 +98
==========================================
+ Hits 2082 2090 +8
- Misses 289 379 +90
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
lib/ex_webrtc/recorder.ex
Outdated
| # XXX need? | ||
| try do | ||
| GenServer.call(recorder, {:add_tracks, tracks}) | ||
| catch | ||
| _exit_or_error, _e -> | ||
| Logger.error("Recorder is down, not adding tracks") | ||
| :error | ||
| end |
There was a problem hiding this comment.
I had the same problem some time ago - link 🙄
IMO, we can remove try catch for now. I don't know how someone could handle the error as it is related to this specific track, not the process of recording as a whole.
Recorder is also assumed to be alive all the time so if it is not, then something bad happened and it might be better for the caller to just crash.
And we can always go back to try catch if we find it useful in the future.
| {:ok, store} -> | ||
| store | ||
|
|
||
| {:error, :late_packet} -> |
There was a problem hiding this comment.
In practice, this should only happen when a recording starts with out-of-order packets, correct?
There was a problem hiding this comment.
Yes, I believe that's correct
| report_path = | ||
| report_path | ||
| |> Path.expand() | ||
| |> then( | ||
| &if(File.dir?(&1), | ||
| do: Path.join(&1, "report.json"), | ||
| else: &1 | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I think we should take a path to the base dir and process all subdirectories. Not necessarily in this PR but in general. Optionally, we can allow to pass an option to process a single dir.
There was a problem hiding this comment.
I agree, but I'd leave it for when we need to use the Converter somewhere (e.g in our demo)
No description provided.