Refactor code around video settings to avoid duplications

[jotaen4tinypilot]

⚠️ Note: this refactoring is blocked on https://github.com/tiny-pilot/tinypilot/issues/1114 (i.e., after the eventual H264 release). This is to avoid significant delays in the H264 roll-out.

---

Currently, we have two video settings: jpegQuality and videoFps. In order for us to get and set the value, and to retrieve the default value, we need (per setting):

• 3 frontend controllers in controllers.js
• 3 backend endpoints in api.py
• 1 request parser plus tests
• 3 property accessors in update.settings

That totals at almost 200 LOC per setting, so 400 LOC for both. The code is practically identical.

I assume we have intentionally opted for duplication and fine-granularity when originally implementing this, because back then it probably wasn’t as clear how the video settings feature would evolve over time.

I think the situation is much clearer now. We are also about to bring in a third parameter h264Bitrate, which will work in the exact same way as the existing two – adding another 200 LOC.

So I think it should be safe for us to invest into a clean-up at this point.

Refactoring goals

• Reduce code duplication. Potential approaches include:
  • Extract common procedures to shared functions
  • Unify the endpoints – we are not selectively setting them anyway, but all video settings are applied together. We could even go as far as setting and applying them in a single operation.
• Simplify naming:
  • Currently, everything is prefixed with video. That’s helpful for the controller functions and the api endpoints, because it acts as a namespace. However, the prefixes are not really necessary within those scopes, e.g. when it comes to the names of the JSON properties or internal variables.
• Establish unified terminology:
  • We recently changed the names of the settings in the frontend: FPS→Frame Rate and JPEG Quality to Quality. We should therefore revise the terminology in the code too:
  • fps → frameRate, because “fps” is just the unit, whereas “frame rate” is “the actual thing”.
  • jpegQuality → mjpegQuality, so that we establish two consistent prefixes mjpeg and h264 for the respective streaming modes. (Reminder: the frame rate setting, in contrast, applies to both streaming modes likewise, so it’s not exclusive to MJPEG, and hence shouldn’t be prefixed.)

Tasks / PRs

(Prioritised)

• [x] Unify endpoints for getting and setting the video parameters.
  • https://github.com/tiny-pilot/tinypilot/pull/1192
• [x] DRY up code around parsing/storing the video settings parameters in the backend, to avoid repetition
  • https://github.com/tiny-pilot/tinypilot/pull/1194
• [x] Rename “fps” and “jpegQuality”
  • https://github.com/tiny-pilot/tinypilot/pull/1198
• [x] Drop the video prefix, unless it acts as “namespace”
  • https://github.com/tiny-pilot/tinypilot/pull/1199

[jotaen4tinypilot]

@mtlynch I slotted this ticket for 2.5.1 as suggestion, because I thought it would make sense to get it done timely (though after all other H264 work in any event). Please feel free to defer. My estimate would be “medium”.

[jotaen4tinypilot]

I’ve updated the issue description with an actionable task list. I’d see most value in the first two items, because they will eliminate a lot of code repetitions; the last two (revising the naming) would be nice for code hygiene, but optional or at least deferable in my eyes.

[mtlynch]

Thanks, @jotaen4tinypilot. I think we should just do all of them this sprint. The last two are deferable, but they also seem like only a straightforward couple hours of work, if that.