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

[James Almer]

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.

> 
>> +static int istg_add(Demuxer *d, AVStreamGroup *stg)
>> +{
>> +    InputFile    *f = &d->f;
>> +    DemuxStreamGroup *dsg;
>> +    const AVBitStreamFilter *filter;
>> +    int base_idx = -1, enhancement_idx = -1;
>> +    int ret;
>> +
>> +    // TODO: generic handling of groups, once support for more is added
>> +    if (stg->type != AV_STREAM_GROUP_PARAMS_LCEVC)
>> +        return 0;
> 
> I'd prefer this function to be essentially a switch that dispatches to
> per-type handlers.

Ok.

> 
>> +
>> +    if (stg->nb_streams != 2)
>> +        return AVERROR_BUG;
>> +
>> +    filter = av_bsf_get_by_name("lcevc_merge");
>> +    if (!filter)
>> +        return 0;
>> +
>> +    dsg = demux_stream_group_alloc(d, stg);
>> +    if (!dsg)
>> +        return AVERROR(ENOMEM);
>> +
>> +    dsg->discard = 1;
>> +
>> +    // set the main stream for the group
>> +    for (int i = 0; i < stg->nb_streams; i++) {
>> +        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.

-------------- 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/3879ac7c/attachment.sig>