Don't just pass 0 for samples in Ogg/FLAC callback

[njlarsson]

This is a fix for issue #661 where I motivated it in a comment, but the comments in the code are actually probably enough to explain it.

[ktmf01]

I see a problem here when very small frames are generated by libFLAC, for example on silence. In that case the write callback isn't called at all, the frame is kept in memory. It seems fixable though.

I suggest adding a variable to the FLAC__OggEncoderAspect struct (defined in ogg_encoder_aspect.h) and then doing the following (I haven't tested this, I assume you have a good way to test this as you actually use this code)

			/* For a batch of write_callback calls, pass the number of samples since the last batch in the first non-metadata
			 * page body call, and then set to zero in case there are more iterations of the while loop (so
			 * as not to give the impression of more samples being processed).
			 */
			aspect->samples_in_submit_buffer += samples;
			while(ogg_stream_pageout(&aspect->stream_state, &aspect->page) != 0) {
				if(write_callback(encoder, aspect->page.header, aspect->page.header_len, 0, current_frame, client_data) != FLAC__STREAM_ENCODER_WRITE_STATUS_OK)
					return FLAC__STREAM_ENCODER_WRITE_STATUS_FATAL_ERROR;
				if(write_callback(encoder, aspect->page.body, aspect->page.body_len, pass_samples, current_frame, client_data) != FLAC__STREAM_ENCODER_WRITE_STATUS_OK)
					return FLAC__STREAM_ENCODER_WRITE_STATUS_FATAL_ERROR;
				aspect->samples_in_submit_buffer = 0;
			}

If you have a better name for the new variable, feel free to change it. Thanks for looking into this, I couldn't find the time to work on this.

[njlarsson]

@ktmf01 Yes, I see how that could be a potential problem, and your suggestion should work (assuming that you meant to replace pass_samples with aspect->samples_in_submit_buffer for the write_callback parameter as well), although handling the case of encoding silence is precisely my reason for looking at the samples parameter in the write callback (I wrote a blog post about it), and it didn’t happen in that case.

I have committed a change based on your suggestion. It works fine in my context. Please tell me if there’s something more I should do – some specific test or something.

[ktmf01]

and it didn’t happen in that case.

Ok. Still, as libogg is an external component we should follow the described behaviour (which is that it is possible a packet is held back) in this case, and not rely on observed behaviour.

I have committed a change based on your suggestion. It works fine in my context. Please tell me if there’s something more I should do – some specific test or something.

I think it is fine. I will take another look at it next week and merge it if I don't think of anything else that needs improvement.

Once again, thanks for picking this up.

[njlarsson]

I noticed another thing when making the change: FLAC__ogg_encoder_aspect_finish just clears the stream. Shouldn’t it call ogg_stream_flush and possibly the write callback in case there is a packet held back? This is a separate issue, though, which should probably be handled separately.

However, when looking into this I also realized that if I understand correctly, the is_metadata case might also involve writing actual samples (again in the case where a packet has been held back), because it calls ogg_stream_flush. Therefore, I moved the aspect->samples_in_submit_buffer logic to affect both branches of the if statement.

[ktmf01]

I just read your blog entry, are you familiar with the 'limit minimum bitrate' function? See https://www.xiph.org/flac/api/group__flac__stream__encoder.html#gac8c5f361b441d528b7a6791b66bb9d40

I'll review your change next week

[njlarsson]

Thanks, yes I had failed to noticed the FLAC__stream_encoder_set_limit_min_bitrate function. I don’t currently seem to need it, but I’ll think about incorporating it.

[ktmf01]

I'm not sure that last commit is necessary: metadata is always written first and samples last, but I don't think it can hurt.

[njlarsson]

In any case, I think the last commit makes the execution more logical and reduces coupling with the calling order. So are you going to merge it?

[ktmf01]

Yes, I'd like to merge it, with "squash and merge". However, I just noticed that the email address you used for your commits is not the same as the address you use for github. Does it matter to you under which email address the commit is credited?

[njlarsson]

No big deal, but the one I use for commits is my general “repository address” also outside of github, and more likely to remain “forever” than the alias I created for the github account.

I’ve used the commit address on the libFLAC mailing list also, BTW,

[njlarsson]

Sorry if I’m confused, but is the change supposed to be merged to master on this repo now? It doesn’t seem to be. Maybe I made a mistake when I created the PR: I was confused about what was left vs right in the Github web interface and may have switched the branches around.

[ktmf01]

The problem is, I prefer to have commits in a PR squashed if there is some back-and-forth or when there are fixups. Usually I just press 'squash and merge' to get the whole PR into one commit, but in that case github takes the github email address for the commit, and not the email address in the commits themselves.

So, yesterday, I squashed your commits by hand, and let the checks run again. Not because it is necessary, but because I prefer it that way. I just merged that squashed commit with 'rebase and merge'.

[njlarsson]

Ok, great, I like squashing too. It was unclear to me that this was what was happening in the Github interface. (The more I learn to love Git, the less I tend to like Github.)

[ktmf01]

Yeah, I agree that github feels a little crude if you are familiar with the finer points of git, like cherry-pick or rebase -i. Still, I prefer the way of organising discussions under PRs to mailing-list patch submission, and the checks (especially CI-fuzz) are very valuable.