google / lyra

A Very Low-Bitrate Codec for Speech Compression
Apache License 2.0
3.84k stars 355 forks source link

Fix to return nullopt when DTX is enabled and audio data is silence #109

Closed sile closed 2 years ago

sile commented 2 years ago

Seeing lyra_encoder.h (see code block below), the Encode method seems to return nullopt if DTX is enabled and the input data contains only silence (noise). However, the current implementation returns an empty vector in the case. This PR fixes the implementation to align with the specification (i.e., the doc in the header file).

  /// Encodes the audio samples into a vector wrapped byte array.
  ///
  /// @param audio Span of int16-formatted samples. It is assumed to contain
  ///              20ms of data at the sample rate chosen at Create time.
  /// @return Encoded packet as a vector of bytes as long as the right amount of
  ///         data is provided. Else it returns nullopt. It also returns nullopt
  ///         if DTX is enabled and the packet is deemed to contain silence.
  std::optional<std::vector<uint8_t>> Encode(
      const absl::Span<const int16_t> audio) override;

Feel free to close this PR if the current implementation is corrent and the doc should be modified.

mchinen commented 2 years ago

Thanks for the PR! I've looked into this and can see that the change was made to empty packets instead of nullopt, but I would like to confirm with the author that it is intentional and that the docs should be updated instead. @astorus-goog would you be able to take a look?

astorus-goog commented 2 years ago

Hi @sile, thank you for bringing this to our attention. It looks like the doc should be updated but the implementation is correct. Since nullopt is meant to signify a recoverable error it should not be used in this case. We will release a fix for the documentation shortly.

sile commented 2 years ago

I see. Thank you for your response!