[FFmpeg-devel] [PATCH 13/13] fftools/ffmpeg_demux: merge streams in a LCEVC stream group

[James Almer]

On 9/6/2024 10:33 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-09-06 14:15:36)
>> On 9/6/2024 8:56 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2024-08-31 18:31:14)
>>>>    static int demux_send(Demuxer *d, DemuxThreadContext *dt, DemuxStream *ds,
>>>>                          AVPacket *pkt, unsigned flags)
>>>>    {
>>>>        InputFile  *f = &d->f;
>>>> -    int ret;
>>>> +    int ret = 0;
>>>>    
>>>>        // pkt can be NULL only when flushing BSFs
>>>>        av_assert0(ds->bsf || pkt);
>>>>    
>>>> +    // a stream can only be disabled if it's needed by a group
>>>
>>> This makes no sense to me.
>>
>> I made av_read_frame() output packets for all the streams belonging to a
>> group, even if some of those streams are not selected/used for an
>> output. In the case of LCEVC groups, the normal scenario is to merge
>> enhancement stream packets into the video one, with the enhancement
>> stream not being used on its own by any output stream.
>> DemuxStream->discard is used for this, where it can be 0 (not used by
>> any output) when AVStream->discard is obviously going to be 1 for lavf
>> to export packets for the stream.
>>
>> If a packet where DemuxStream->discard is 0 reaches this point, the only
>> valid scenario is that it's part of a group.
>>
>>>
>>>> +    av_assert0(ds->nb_stream_groups || !ds->discard);
>>>> +
>>>> +    // create a reference for the packet to be filtered by group bsfs
>>>
>>> What are "group bsfs"?
>>
>> Bsfs that are used by and all (or some) of a group's streams, as is the
>> case of lcevc_merge, and stored in DemuxStreamGroup->bsf.
>>
>>>
>>>> +    if (pkt && ds->nb_stream_groups) {
>>>> +        av_packet_unref(dt->pkt_group_bsf);
>>>> +        ret = av_packet_ref(dt->pkt_group_bsf, pkt);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>>        // send heartbeat for sub2video streams
>>>> -    if (d->pkt_heartbeat && pkt && pkt->pts != AV_NOPTS_VALUE) {
>>>> +    if (d->pkt_heartbeat && pkt && !ds->discard && pkt->pts != AV_NOPTS_VALUE) {
>>>
>>> Random added checks for ds->discard are extremely confusing and tell me
>>> you're overloading that poor field to mean something extremely
>>> non-obvious.
>>
>> I added this here to make sure I'm not sending a heartbeat packet when
>> handling a packet for a stream that's not used by any output on its own.
> 
> This is not the the only place where you're adding a check for discard,

Because it's the only code i don't want to trigger for non-standalone 
output packets.

> and I strongly dislike that 'discard' now does not mean 'discard'
> anymore.

No, the packet is discarded in the end, and never makes anywhere on its 
own unless it's used by an output. DemuxStream->discard is in fact the 
one field where you can check if a stream is effectively disabled, and 
that doesn't change with this patch. What changes is that lavf may now 
output packets not used by any output (AVStream->discard being 0), and 
the code needs to be aware of this.

> 
> Furthermore, I really dislike how invasive such an obscure feature is.

Ideally, lavf would export the LCEVC stream payload as packet side data 
in the video's stream, but that's apparently only possible with mov/mp4 
and maybe matroska, not TS. Hence a Stream Group is used and everything 
left to the caller.

That said, is this so invasive? This patch adds 
DemuxStreamGroup/InputStreamGroup, cleanly initializes them like we 
already do with DemuxStream/InputStream, and then factorizes the bsf 
packet loop so it can be used for the stream bsf and the group bsf.
The only truly big change is making libavformat output streams that are 
not strictly used by an output, which is achieved by making 
AVStream->discard not be set, and keep relying on DemuxStream->discard 
to know what stream is disabled or not.

You can try remuxing an LCEVC sample with split enhancement in different 
ways to test this:

# decode video stream only. Enhancement stream is output as well and 
merged into video stream as side data, which lavc will see and use.
ffmpeg -i input.mp4 output.mp4

# remux video stream only. Enhancement stream is output as well and 
merged into video stream as side data. Muxers will not care about it, 
but an hypothetical bsf could for example be inserted in the process to 
add it to the h264/hevc bitstream as a SEI message.
ffmpeg -i input.mp4 -c:v copy -dn -map 0:0 output.mp4

# remux enhancement stream only. Video stream is output by lavf but 
discarded by the CLI.
ffmpeg -i input.mp4 -c:d copy -vn -map 0:1 output.mp4

# remux both video and enhancement streams. Enhancement stream is merged 
into video stream as side data too.
ffmpeg -i input.mp4 -c:v copy -c:d copy -map 0 output.mp4

> 
>>>> +        int j;
>>>> +
>>>> +        for (j = 0; j < f->nb_streams; j++)
>>>> +            if (stg->streams[i] == f->streams[j]->st)
>>>> +                break;
>>>> +
>>>> +        if (j == f->nb_streams)
>>>> +            return AVERROR_BUG;
>>>
>>> Isn't all this just "j = stg->streams[i]->index"?
>>
>> Is f->streams guaranteed to have the same streams as ic->streams? If so,
>> probably. Will change then.
> 
> I guess you're right that it's better not to rely on it. Still, I'd
> rather this was factored into a separate function.

Ok.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240906/e23dac5c/attachment.sig>