search

ralfbiedert / openh264-rs

Idiomatic Rust wrappers around OpenH264.
66 stars 32 forks source link

feat: add encode_at method for encoding frame at a specific timestamp #41

Closed shelbyd closed 8 months ago

ralfbiedert commented 8 months ago

Thanks!

ralfbiedert commented 8 months ago

I've changed the parameter to u64 since Duration didn't feel right. It's a timestamp, but Instant also doesn't quite have the constructors to be used easily. Do you agree?

shelbyd commented 8 months ago

I definitely recommend Duration. The actual value is milliseconds since beginning of the file. And Duration represents the idea of an amount of time.

Duration makes it pretty clear to users how to call the method correctly. While with just u64 you need to document that it is millis. Additionally, there are a lot of utilities around Duration and Instant, and converting to/from in user code is annoying.

It would be nice to use Instant, but Instant represents an actual monotonic wall clock time. So the use case doesn't line up.

The best option would be something like a relative Instant? But that is equivalent to Duration in a lot of ways.

ralfbiedert commented 8 months ago

I definitely recommend Duration. The actual value is milliseconds since beginning of the file. And Duration represents the idea of an amount of time.

The reason I don't like Duration is that I find it confusing what that duration means in this context. Is it duration since start of file, or duration since the last frame?

Duration makes it pretty clear to users how to call the method correctly. While with just u64 you need to document that it is millis.

I agree having a strong type would be much nicer, and we should probably have our own timestamp type with some from_seconds_f32(), from_micros, ... ctors.

Additionally, there are a lot of utilities around Duration and Instant, and converting to/from in user code is annoying.

Maybe I misunderstand, but wouldn't any type have some conversion friction?

Totally unrelated, I just added a test case if the timestamp roundtrips. I'm not sure if I did something wrong, but when I feed in 64 into the encoder, I still get 0 in the decoder. It would be nice if you could create a follow-up PR that gets this test case to work.