Skip to content

Encoder/Decoder initialization method#525

Open
dl1hrc wants to merge 2 commits into
sm0svx:masterfrom
dl1hrc:coder-init
Open

Encoder/Decoder initialization method#525
dl1hrc wants to merge 2 commits into
sm0svx:masterfrom
dl1hrc:coder-init

Conversation

@dl1hrc

@dl1hrc dl1hrc commented Dec 6, 2020

Copy link
Copy Markdown
Contributor

Hi Tobias,
would be nice if you could check my version of initialization of Encoder/Decoder objects.
This variant transfers all necessary parameters immediately for the initialization of the decoder / encoder and not only when the object has been instantiated via the setOption() method. This is the prerequisite for extensions, e.g. based on hardware that take on both encoder and decoder functions (e.g. Dv3k)
mni 73s de Adi / DL1HRC

…nitialization of the decoder / encoder and not only when the object has been instantiated via the setOption() method. This is the prerequisite for extensions, e.g. based on hardware that take on both encoder and decoder functions (e.g. Dv3k)

@MarkRose MarkRose left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same AI-assisted review pass, now on this one. The main thing it flagged looks like a crash regression in the Opus encoder when no frame-size option is given; there's also a small header-consistency nit. Details inline.

setBitrate(20000);
enableVbr(true);
Options::const_iterator it;
for (it=options.begin(); it!=options.end(); it++)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the ctor now driving everything through this options loop, the previous unconditional setFrameSize(20) is gone — and setFrameSize is the only place sample_buf is allocated (new float[frame_size]). Members start at frame_size(0), sample_buf(0), so if the encoder is created without a FRAME_SIZE option (e.g. create("OPUS", {}) or a config without OPUS_ENC_FRAME_SIZE), sample_buf stays null and writeSamples does sample_buf[buf_len++] = … through a null pointer → crash. Keeping setFrameSize(20); setBitrate(20000); enableVbr(true); before the options loop would preserve the old default and still let options override.

* @brief Default constuctor
*/
AudioDecoderSpeex(void);
AudioDecoderSpeex(const Options &options);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small consistency nit: AudioDecoderOpus got = Options() on its new ctor, but AudioDecoderSpeex/AudioEncoderSpeex/AudioEncoderOpus didn't, so code still constructing those with the old zero-arg form won't compile. Defaulting all four (or none) keeps it consistent and avoids breaking out-of-tree callers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants