[FFmpeg-devel] [PATCH] Add picture attachment support to OGG Muxer

[LRFLEW]

>seems to break fate



Ok, I spent some time looking into what's happening, and I've figured what's causing the failures. The issue isn't that it's producing *invalid* output, but that it's just *different* output. As I had said, I wrote this patch some time ago, so I had forgotten a decision I had made to work around a problem I was facing. The way I implemented the delayed writing of the Vorbis Comments meant that the comments packet needed to end the OGG page. This was already the case for the audio codecs (which is what I was writing it to use it for at the time), but the video codecs have another header packet after the vorbis comments that was being written to the same page (if it fit). I ended up splitting the page so that the headers were placed in different pages. You can see this in the patch where, in the second loop in ogg_write_header, I replaced one call to ogg_buffer_page with two.


The way I see it, there are three options here:


* Accept that the new output is valid and update the test cases
* Rework the way the Vorbis Comments are written to avoid the issue

* Use an if statement to only split the headers into separate pages when there are attachments


The last one is the quickest to implement, as I have already done it to test that this is the only thing causing fate errors. However, it very much feels like a hack, changing the code specifically for a test case instead of either fixing the test case or solving the underlying problem. The first option is the next easiest, as it would just be a matter of updating the ref files. However, I can see that being an unpopular option, depending on how much the change in output is considered an "issue" (as apposed to just a change). The middle one is probably the most complete, but would be a real challenge. Having been thinking about it, I think it might be reasonable to implement if, instead of buffering the header pages when ogg_write_header is called, the buffering/writing of the pages is delayed until all the attachments are ready, using OGGStreamContext.header to store/buffer the data until it's ready to be written.


Does anybody have any feedback on this? I'll probably start experimenting with the second option, but it might take me a while before I have anything functional for it.