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

["Dawid Kozinski/Multimedia \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics" <d.kozinski@samsung.com>]

> -----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.

BR
Dawid

> 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?
> 
> Thanks,
> 
> - Mark
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://protect2.fireeye.com/v1/url?k=fe563622-a1cdf846-fe57bd6d-
> 000babda0201-8b2ed12312438c45&q=1&e=8a63da42-a031-4561-b197-
> 0dcaaf458058&u=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmp
> eg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".


_______________________________________________
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".