[FFmpeg-devel] Question regarding ogg cover art implementation

[Chad Fraleigh]

On 8/22/2021 4:49 PM, Jesse Obic wrote:

> I'm looking at implementing https://trac.ffmpeg.org/ticket/4448 
> <https://trac.ffmpeg.org/ticket/4448>, and I understand what I need to 
> do but I'm on the fence about how I should go about doing it.
> 
> For the OGG container, cover art is embedded into the file by adding a 
> METADATA_BLOCK_PICTURE tag in the vorbis comment section. The data of 
> this tag is a base64 representation of the FLAC embedded image data 
> structure, which includes the data of the image itself.
> 
> I looked at how vorbis comments are currently written, and it's lead me 
> to `ff_vorbiscomment_write`. It takes in an AVDictionary for metadata, 
> and an array of AVChapters for chapter writing. This is where I get a 
> bit concerned.
> 
> I see two options as to how I could handle this:
> 
> 
> 1. Place the METADATA_BLOCK_PICTURE tag in the metadata AVDictionary.
> 
> This would be the easiest functionality wise, however it feels like a 
> bad idea in my gut because the dictionary would be housing a value that 
> is 1.33 * the size of the image(s) being embedded. So for example, if 
> you wished to embed a 12 MB image it would require a 16MB malloc which 
> doesn't feel right.

Would allocating 16M really be a lot, given typically there wouldn't be 
many of them?


> 2. Change ff_vorbiscomment_write to take in an array of AVStream / 
> AVPacket to write it directly to the output IO stream
> 
> As far as I know this would classify as an API/ABI change. However it 
> would mean that I don't have to allocate such a huge array upfront, and 
> can encode smaller chunks at a time as I write them to the output stream.
> 
> To mitigate it, it could be created as a new function with the 
> additional parameters, and the old function pointing towards the new one 
> instead. I'm not entirely sure how APIs / ABIs are exposed and consumed 
> in C, so I'm not sure if this will fix the issue.

What if.. LOB value reference support was added to AVDictionary, via a 
av_dict_set_lob() function and AVDictionaryEntry->lob field. And anytime 
av_dict_get() is called without a AV_DICT_NO_RESOLVE flag, it would 
simply do a lazy resolve if it lob != NULL and fill in value field (if 
not already resolved, of course). This would allow a LOB aware callers 
to stream it when it is a LOB (use 'value' when not NULL). It should 
even be API/ABI change safe (unless making AVDictionaryEntry bigger 
counts as a break).

Anyway.. just a third option.


Oh, and on a side note: av_dict_get() should _probably_ be changed to 
return a const AVDictionaryEntry * at some point, since it is internal 
and really should never be modified by the caller.