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

[Anton Khirnov]

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,
and I strongly dislike that 'discard' now does not mean 'discard'
anymore.

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

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

-- 
Anton Khirnov