Re: [FFmpeg-devel] [PATCH v1 5/8] avformat/mov_muxer: Extended MOV muxer to handle APV video content

[Mark Thompson <sw@jkqxz.net>]

On 24/04/2025 13:08, Dawid Kozinski/Multimedia (PLT) /SRPOL/Staff Engineer/Samsung Electronics wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: środa, 23 kwietnia 2025 23:08
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v1 5/8] avformat/mov_muxer: Extended
>> MOV muxer to handle APV video content
>>
>> On 23/04/2025 15:13, Dawid Kozinski wrote:
>>> - Changes in mov_write_video_tag function to handle APV elementary
>>> stream
>>> - Provided structure APVDecoderConfigurationRecord that specifies the
>>> decoder configuration information for APV video content
>>>
>>> Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
>>> ---
>>>  libavformat/Makefile    |   2 +-
>>>  libavformat/apv.c       | 827 ++++++++++++++++++++++++++++++++++++++++
>>>  libavformat/apv.h       |  94 +++++
>>>  libavformat/isom_tags.c |   2 +
>>>  libavformat/movenc.c    |  47 +++
>>>  5 files changed, 971 insertions(+), 1 deletion(-)
>>
>> Hi,
>>
>> Two thoughts here:
>>
>> First, your AVPackets contain a raw_bitstream_access_unit().  I don't
> think this is
>> the right approach - the packets should contain the codec data only, not
> the
>> additional encapsulation.  (This is the method I followed.)
>>
>> For this patch in particular, I think it results in writing the files
> incorrectly: the
>> specification says "each sample shall contain one and only one access unit
> of
>> APV coded data", which I interpret to mean one access_unit() syntax
> structure.
>>
>> This also results in the size effectively appearing multiple times in the
> file for no
>> good reason:
>>
>> 00000020  66 72 65 65 00 01 15 db  6d 64 61 74 00 01 15 cf
> |free....mdat....|
>>
>>                       ^ mdat size              ^ au_size
>>
>> 00000030  61 50 76 31 00 01 15 c7  01 00 01 00 21 21 40 00
> |aPv1........!!@.|
>>           ^ signature ^ pbu_size   ^ pbu_type followed by header
>>
>> The separate pbu_size makes sense if there is also metadata, but having
> the
>> mdat box with a size immediately followed by the same size (well, minus
> twelve
>> for mdat size + mdat + au size) again inside the box does not seem
> helpful.
>>
> 
> Hi Mark,
> 
> Indeed. AVPackets contains raw_bitstream_access_unit()
> 
> raw_bitstream_access_unit() {                                  
>     au_size                                                   | u(32)
>     access_unit(au_size)                                      |
> }         
> 
> Such data comes out from the APV encoder.
> Data from the encoder can go to the movmuxer, but it can also go to another
> muxer like apvmuxer.
> 
> const FFOutputFormat ff_apv_muxer = {
>     .p.name            = "apv",
>     .p.long_name       = NULL_IF_CONFIG_SMALL("raw APV video"),
>     .p.extensions      = "apv",
>     .p.audio_codec     = AV_CODEC_ID_NONE,
>     .p.video_codec     = AV_CODEC_ID_APV,
>     .flags_internal    = FF_OFMT_FLAG_MAX_ONE_OF_EACH |
>                          FF_OFMT_FLAG_ONLY_DEFAULT_CODECS,
>     .write_packet      = ff_raw_write_packet,
>     .p.flags           = AVFMT_NOTIMESTAMPS,
> };
> 
> Thanks to the fact that the APV muxer receives such data packed in AVPacket,
> I was able to use the default function ff_raw_write_packet() from rawenc.c
> as FFOutputFormat::write_packet for ff_apv_muxer.
> 
> The APV muxer uses ff_raw_write_packet, which takes the data that came from
> the encoder packed in AVPacket and writes the data to a file.
> 
> In the case of saving the elementary APV stream to a file, the data should
> have the format [au_size (4Bytes)][access_unit][au_size
> (4Bytes)][access_unit]...[au_size(4Bytes)][access_unit].
> 
> Regarding APV in movmuxer, we can eliminate the additional AU size in two
> ways:
> 
> 1.
> We can change the output data format from the encoder so that the AVPacket
> contains access_unit() without the 4-byte prefix containing information
> about the length of the AU. However, this will require using a custom
> .write_packet function in ff_apv_muxer that will add a prefix specifying the
> size of the AU in the output stream before each access unit.
> 
> 
> 2. 
> We will make changes in the mov muxer (movenc.c). We will not write the data
> using the call avio_write(pb, pkt->data, size); but will implement custom
> handling for APV (an additional if() in ff_mov_write_packet()):
> 
> if (par->codec_id == AV_CODEC_ID_APV) {
>     avio_write(pb, pkt->data + 4, size - 4);
> }
> 
> Please share your opinion.

I don't think the implementation details which you mention are relevant to the questions I am asking.


The primary point here is that the spec at <https://github.com/AcademySoftwareFoundation/openapv/blob/main/readme/apv_isobmff.md> says:

  "each sample shall contain one and only one access unit of APV coded data"

This states that it contains an "access unit", not a "raw bitstream access unit" as in your implementation.

Since your implementation and the spec disagree, can you comment on the status / intended finality of each of them?  (Is there another implementation of this which you are testing against, or is this the first one?)

To me the spec definition appears to be the more sensible one, since the raw bitstream format functions as a container and it does not make sense to nest containers.


Separately to that queston, there is the point that inside ffmpeg we may make our own choice about what an AVPacket should contain (and then use it consistently).

In this case my opinion is that having an AVPacket contain an access unit (the top-level syntax structure defined in the "syntax and semantics" section of the specification) is the choice which makes most sense, and it is also the one which I have followed consistently in my patch series.

>> Second, I think we need a consistent decision on what the extradata should
> be
>> doing.  The APVDecoderConfigurationRecord makes sense as a thing for it to
>> contain, but it's not clear to me that it needs to exist at all as it has
> no effect on
>> anything inside ffmpeg (a decoder will always ignore it).
>>
>> You currently make extradata from one of your demuxers but not other one
> or
>> the encoder, and nothing requires it when consuming.  Why is it useful to
> have
>> ever?

Your comment on this question would also be appreciated.

Thanks,

- Mark

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".