append_encode_mux! -> Base.write, and simplify

[IanButterworth]

master

open_video_out("video.mp4", first(framestack)) do writer
    for i in eachindex(framestack)
        append_encode_mux!(writer, framestack[i], i)
    end
end

this PR

open_video_out("video.mp4", first(framestack)) do writer
    for frame in framestack
        write(writer, frame)
    end
end

Given the index had to start at 0, and increment monotonically, we might as well do it internally

Edit: was push! now is write

[codecov[bot]]

Codecov Report

Merging #307 (9032318) into master (7a3bb9a) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #307   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files          17       17           
  Lines        1198     1198           
=======================================
  Hits          987      987           
  Misses        211      211           

Impacted Files	Coverage Δ
src/VideoIO.jl	53.33% <ø> (ø)
src/encoding.jl	93.20% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7a3bb9a...9032318. Read the comment docs.

[galenlynch]

I haven't looked at the code yet but from just reading your description I think that internally setting index and removing it from the API might cause problems down the road. index is a stand-in for FFmpeg's pts or presentation timestamp, which I think must be manually set by users if they were going to actually use the simultaneous muxing and writing capability added in #279 to encode multiple media streams (e.g. adding audio), or instead encode video data that does not have a constant framerate (such as from a webcam). While neither of these possibilities are currently supported, I think they're obvious extensions that would be nice to add to VideoIO. Being able to manually set index will be important in both of these cases, and also for "power" users that might want to add an audio track with the ffmpeg binary at a particular offset from the video data, or instead have blank frames before the start of a video without adding video data.

In terms of naming, I just used append_encode_mux! to distinguish this function from the existing appendencode!, and you're right that it's not the best name. It would be nice to use a name in Base, like push!, however I think push!feels a little bit off since VideoWriter is not a collection (i.e. you cannot access past frames with it), and the function is basically writing the video frame to the video container. Maybe something in the write family would be better? I don't really have a strong preference either way, and will defer to you if you like push!.

Thanks for doing this! I think whatever we name it will be a big usability and discoverability win.

[IanButterworth]

Yeah, I think you're right. Base.write(writer::VideoWriter, img) makes a bit more sense.

I think if users want to do more low-level things, they wouldn't be using append_encode_mux!. So I'll move the auto handling of index to within that function, so that we don't interfere with lower level use.

[galenlynch]

Yeah I just think that there are reasons why users might want to set pts manually, which will be even more true if VideoIO gains the ability to encode variable frame rate videos (which shouldn't be too hard). Maybe instead of removing the index argument from the top-level function you could give it a default (e.g. nothing) which will use the auto-increment value, while still allowing users to set it manually if they want?

[IanButterworth]

Instead of providing a default of nothing, I just provided two write methods, one with index and one without. I guess it may be more efficient than doing an isnothing check(?) Edit: I checked, they're about the same

[IanButterworth]

@galenlynch are you ok with me merging this?

[galenlynch]

yep!