the updated liboggedit can't write tags to ogg vorbis file

[Former user]

I looked through the diff, and this line is the culprit:

(in copy_remaining_pages)

while (serial > OGGEDIT_EOF && is_data_page(&og, codec_serial, serial));

if I change it back to this, it works:

while (serial > OGGEDIT_EOF && serial == codec_serial && ogg_page_granulepos(&og) <= 0);

[Alexey Yakovenko]

Err.. I thought I logged in, but the issue still got created as anonymous. Sorry.

[Alexey Yakovenko]

This is the change that I had to apply: https://github.com/DeaDBeeF-Player/deadbeef/commit/79d211fe18d838d448071abfabf5b95a29f8e8ab

[Alexey Yakovenko]

I think I see what's wrong with the code now.. will make a better fix.

[Alexey Yakovenko]

This patch also fixes the problem.. do you think it's correct?

 static bool is_data_page(ogg_page *og, int64_t codec_serial, int64_t serial)
 {
-    return ogg_page_granulepos(og) != 0 && serial == codec_serial;
+    return ogg_page_granulepos(og) <= 0 && serial == codec_serial;
 }

[Ian Nartowicz]

The granulepos is effectively the last sample on a page, or 0 for a header page. ogg_page_granulepos returns a signed int64, so it may return a negative value for very large sample positions or for pages which do not contain the end of a packet (special value 2^64 - 1). So the test for a data page should really be "!= 0", but "<= 0" works in almost all cases. Hence the changes I made.

I think is_data_page() is wrong, and not just because it tests whether the page is NOT a data page. The diff you uploaded to deadbeef looks OK (except for the very rare cases) because it is just what was there before. The patch to is_data_page() shown here doesn't look right. I don't think a function is_data_page() cannot be used at all and the two callers need to each have their own logic. I'll upload something here when I've had chance to test.

I don't think your errors are anything to do with the granulepos though, because the error is with the serialno check. Do you have chained files? Multiplexed files?

[Alexey Yakovenko]

I probably have a couple of chained vorbis files, but the ones I tested were just regular ones.

[Ian Nartowicz]

I've pushed what I think this should be. It would help if you could send me the vorbis file you are having problems with. I've tested against my opus files and a couple of vorbis tracks, but there must be something in your file that I haven't seen.

More generally, this liboggedit copy doesn't include oggedit_vorbis (or oggedit_flac), so merging it into deadbeef may produce unexpected problems. I changed quite a few data types in oggedit_opus, which should probably be replicated in oggedit_vorbis. I think most of them were for largefile support on 32 bit machines, so maybe not too important.

[Alexey Yakovenko]

email sent

[Alexey Yakovenko]

I changed quite a few data types in oggedit_opus, which should probably be replicated in oggedit_vorbis

ohh.. I didn't realize that. but it builds fine. I will double-check any warnings though.

[Alexey Yakovenko]

I validated fd9333b, it works.

[Alexey Yakovenko]

(the issue can be closed)

[Ian Nartowicz]

I think I know why I wasn't seeing this issue, and it is probably something you should check in your testing too. liboggedit adds padding to the metadata pages so that most changes can be written in place. Without the padding, the metadata size changes and the whole file has to be re-written. The bug only occurs when the whole file is re-written. Once you make a successful edit to the metadata then there will be some padding and you won't hit the buggy code again with that file unless you make a huge change to the metadata. Most of my test files have been edited before and I just wasn't hitting that code.