[FFmpeg-devel] avformat/flvenc: avoid an extra allocate

From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavformat/flvenc.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

On 4/6/2023 5:11 PM, Zhao Zhili wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> ---
>   libavformat/flvenc.c | 24 +++++++-----------------
>   1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> index a7b5efde4b..3a686b73d7 100644
> --- a/libavformat/flvenc.c
> +++ b/libavformat/flvenc.c
> @@ -117,12 +117,9 @@ typedef struct FLVContext {
>       AVCodecParameters *data_par;
>   
>       int flags;
> +    uint32_t last_ts[3];

I understand that timestamps are 32bit on flv, but things are pretty 
inconsistent type wise across the entire file. This used to be int64_t 
and you're changing it to uint32_t. Some parameters expect an unsigned 
int and others an int64_t. It would be nice to choose a single type and 
use it everywhere.
The source for all of them is apparently pkt->dts, which is int64_t, so 
I'd use that everywhere and ensure its value is <= INT32_MAX or <= 
UINT32_MAX depending on how you store it.

>   } FLVContext;
>   
> -typedef struct FLVStreamContext {
> -    int64_t last_ts;    ///< last timestamp for each stream
> -} FLVStreamContext;
> -
>   static int get_audio_flags(AVFormatContext *s, AVCodecParameters *par)
>   {
>       int flags = (par->bits_per_coded_sample == 16) ? FLV_SAMPLESSIZE_16BIT
> @@ -611,7 +608,7 @@ static int flv_init(struct AVFormatContext *s)
>   
>       for (i = 0; i < s->nb_streams; i++) {
>           AVCodecParameters *par = s->streams[i]->codecpar;
> -        FLVStreamContext *sc;
> +
>           switch (par->codec_type) {
>           case AVMEDIA_TYPE_VIDEO:
>               if (s->streams[i]->avg_frame_rate.den &&
> @@ -675,12 +672,7 @@ static int flv_init(struct AVFormatContext *s)
>               return AVERROR(EINVAL);
>           }
>           avpriv_set_pts_info(s->streams[i], 32, 1, 1000); /* 32 bit pts in ms */
> -
> -        sc = av_mallocz(sizeof(FLVStreamContext));
> -        if (!sc)
> -            return AVERROR(ENOMEM);
> -        s->streams[i]->priv_data = sc;
> -        sc->last_ts = -1;
> +        flv->last_ts[i] = -1;
>       }
>   
>       flv->delay = AV_NOPTS_VALUE;
> @@ -783,10 +775,9 @@ end:
>           /* Add EOS tag */
>           for (i = 0; i < s->nb_streams; i++) {
>               AVCodecParameters *par = s->streams[i]->codecpar;
> -            FLVStreamContext *sc = s->streams[i]->priv_data;
>               if (par->codec_type == AVMEDIA_TYPE_VIDEO &&
>                       (par->codec_id == AV_CODEC_ID_H264 || par->codec_id == AV_CODEC_ID_MPEG4))
> -                put_eos_tag(pb, sc->last_ts, par->codec_id);
> +                put_eos_tag(pb, flv->last_ts[i], par->codec_id);
>           }
>       }
>   
> @@ -821,7 +812,6 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
>       AVIOContext *pb      = s->pb;
>       AVCodecParameters *par = s->streams[pkt->stream_index]->codecpar;
>       FLVContext *flv      = s->priv_data;
> -    FLVStreamContext *sc = s->streams[pkt->stream_index]->priv_data;
>       unsigned ts;
>       int size = pkt->size;
>       uint8_t *data = NULL;
> @@ -919,13 +909,13 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
>       }
>   
>       /* check Speex packet duration */
> -    if (par->codec_id == AV_CODEC_ID_SPEEX && ts - sc->last_ts > 160)
> +    if (par->codec_id == AV_CODEC_ID_SPEEX && ts - flv->last_ts[pkt->stream_index] > 160)
>           av_log(s, AV_LOG_WARNING, "Warning: Speex stream has more than "
>                                     "8 frames per packet. Adobe Flash "
>                                     "Player cannot handle this!\n");
>   
> -    if (sc->last_ts < ts)
> -        sc->last_ts = ts;
> +    if (flv->last_ts[pkt->stream_index] < ts)
> +        flv->last_ts[pkt->stream_index] = ts;
>   
>       if (size + flags_size >= 1<<24) {
>           av_log(s, AV_LOG_ERROR, "Too large packet with size %u >= %u\n",

On Thu, 2023-04-06 at 11:25 -0300, James Almer wrote:
> On 4/6/2023 5:11 PM, Zhao Zhili wrote:
> > From: Zhao Zhili <zhilizhao@tencent.com>
> > 
> > ---
> >   libavformat/flvenc.c | 24 +++++++-----------------
> >   1 file changed, 7 insertions(+), 17 deletions(-)
> > 
> > diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> > index a7b5efde4b..3a686b73d7 100644
> > --- a/libavformat/flvenc.c
> > +++ b/libavformat/flvenc.c
> > @@ -117,12 +117,9 @@ typedef struct FLVContext {
> >       AVCodecParameters *data_par;
> >   
> >       int flags;
> > +    uint32_t last_ts[3];
> 
> I understand that timestamps are 32bit on flv, but things are pretty 
> inconsistent type wise across the entire file. This used to be int64_t 
> and you're changing it to uint32_t. Some parameters expect an unsigned 
> int and others an int64_t. It would be nice to choose a single type and 
> use it everywhere.
> The source for all of them is apparently pkt->dts, which is int64_t, so 
> I'd use that everywhere and ensure its value is <= INT32_MAX or <= 
> UINT32_MAX depending on how you store it.

I can change it to int64_t. It worths to note that all usecases of it are
unsigned (the type of 'ts', the prototype of put_eos_tag()).

> 
> >   } FLVContext;
> >   
> > -typedef struct FLVStreamContext {
> > -    int64_t last_ts;    ///< last timestamp for each stream
> > -} FLVStreamContext;
> > -
> >   static int get_audio_flags(AVFormatContext *s, AVCodecParameters *par)
> >   {
> >       int flags = (par->bits_per_coded_sample == 16) ? FLV_SAMPLESSIZE_16BIT
> > @@ -611,7 +608,7 @@ static int flv_init(struct AVFormatContext *s)
> >   
> >       for (i = 0; i < s->nb_streams; i++) {
> >           AVCodecParameters *par = s->streams[i]->codecpar;
> > -        FLVStreamContext *sc;
> > +
> >           switch (par->codec_type) {
> >           case AVMEDIA_TYPE_VIDEO:
> >               if (s->streams[i]->avg_frame_rate.den &&
> > @@ -675,12 +672,7 @@ static int flv_init(struct AVFormatContext *s)
> >               return AVERROR(EINVAL);
> >           }
> >           avpriv_set_pts_info(s->streams[i], 32, 1, 1000); /* 32 bit pts in ms */
> > -
> > -        sc = av_mallocz(sizeof(FLVStreamContext));
> > -        if (!sc)
> > -            return AVERROR(ENOMEM);
> > -        s->streams[i]->priv_data = sc;
> > -        sc->last_ts = -1;
> > +        flv->last_ts[i] = -1;
> >       }
> >   
> >       flv->delay = AV_NOPTS_VALUE;
> > @@ -783,10 +775,9 @@ end:
> >           /* Add EOS tag */
> >           for (i = 0; i < s->nb_streams; i++) {
> >               AVCodecParameters *par = s->streams[i]->codecpar;
> > -            FLVStreamContext *sc = s->streams[i]->priv_data;
> >               if (par->codec_type == AVMEDIA_TYPE_VIDEO &&
> >                       (par->codec_id == AV_CODEC_ID_H264 || par->codec_id == AV_CODEC_ID_MPEG4))
> > -                put_eos_tag(pb, sc->last_ts, par->codec_id);
> > +                put_eos_tag(pb, flv->last_ts[i], par->codec_id);
> >           }
> >       }
> >   
> > @@ -821,7 +812,6 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
> >       AVIOContext *pb      = s->pb;
> >       AVCodecParameters *par = s->streams[pkt->stream_index]->codecpar;
> >       FLVContext *flv      = s->priv_data;
> > -    FLVStreamContext *sc = s->streams[pkt->stream_index]->priv_data;
> >       unsigned ts;
> >       int size = pkt->size;
> >       uint8_t *data = NULL;
> > @@ -919,13 +909,13 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
> >       }
> >   
> >       /* check Speex packet duration */
> > -    if (par->codec_id == AV_CODEC_ID_SPEEX && ts - sc->last_ts > 160)
> > +    if (par->codec_id == AV_CODEC_ID_SPEEX && ts - flv->last_ts[pkt->stream_index] > 160)
> >           av_log(s, AV_LOG_WARNING, "Warning: Speex stream has more than "
> >                                     "8 frames per packet. Adobe Flash "
> >                                     "Player cannot handle this!\n");
> >   
> > -    if (sc->last_ts < ts)
> > -        sc->last_ts = ts;
> > +    if (flv->last_ts[pkt->stream_index] < ts)
> > +        flv->last_ts[pkt->stream_index] = ts;
> >   
> >       if (size + flags_size >= 1<<24) {
> >           av_log(s, AV_LOG_ERROR, "Too large packet with size %u >= %u\n",
> _______________________________________________
> 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".

On 4/6/2023 1:47 PM, Zhao Zhili wrote:
> On Thu, 2023-04-06 at 11:25 -0300, James Almer wrote:
>> On 4/6/2023 5:11 PM, Zhao Zhili wrote:
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>
>>> ---
>>>    libavformat/flvenc.c | 24 +++++++-----------------
>>>    1 file changed, 7 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
>>> index a7b5efde4b..3a686b73d7 100644
>>> --- a/libavformat/flvenc.c
>>> +++ b/libavformat/flvenc.c
>>> @@ -117,12 +117,9 @@ typedef struct FLVContext {
>>>        AVCodecParameters *data_par;
>>>    
>>>        int flags;
>>> +    uint32_t last_ts[3];
>>
>> I understand that timestamps are 32bit on flv, but things are pretty
>> inconsistent type wise across the entire file. This used to be int64_t
>> and you're changing it to uint32_t. Some parameters expect an unsigned
>> int and others an int64_t. It would be nice to choose a single type and
>> use it everywhere.
>> The source for all of them is apparently pkt->dts, which is int64_t, so
>> I'd use that everywhere and ensure its value is <= INT32_MAX or <=
>> UINT32_MAX depending on how you store it.
> 
> I can change it to int64_t. It worths to note that all usecases of it are
> unsigned (the type of 'ts', the prototype of put_eos_tag()).

put_timestamp() takes an int64_t, which gets pkt->dts as argument from 
flv_write_codec_header() and flv_write_packet(). They should all be 
int64_t and range checked.

> 
>>
>>>    } FLVContext;
>>>    
>>> -typedef struct FLVStreamContext {
>>> -    int64_t last_ts;    ///< last timestamp for each stream
>>> -} FLVStreamContext;
>>> -
>>>    static int get_audio_flags(AVFormatContext *s, AVCodecParameters *par)
>>>    {
>>>        int flags = (par->bits_per_coded_sample == 16) ? FLV_SAMPLESSIZE_16BIT
>>> @@ -611,7 +608,7 @@ static int flv_init(struct AVFormatContext *s)
>>>    
>>>        for (i = 0; i < s->nb_streams; i++) {
>>>            AVCodecParameters *par = s->streams[i]->codecpar;
>>> -        FLVStreamContext *sc;
>>> +
>>>            switch (par->codec_type) {
>>>            case AVMEDIA_TYPE_VIDEO:
>>>                if (s->streams[i]->avg_frame_rate.den &&
>>> @@ -675,12 +672,7 @@ static int flv_init(struct AVFormatContext *s)
>>>                return AVERROR(EINVAL);
>>>            }
>>>            avpriv_set_pts_info(s->streams[i], 32, 1, 1000); /* 32 bit pts in ms */
>>> -
>>> -        sc = av_mallocz(sizeof(FLVStreamContext));
>>> -        if (!sc)
>>> -            return AVERROR(ENOMEM);
>>> -        s->streams[i]->priv_data = sc;
>>> -        sc->last_ts = -1;
>>> +        flv->last_ts[i] = -1;
>>>        }
>>>    
>>>        flv->delay = AV_NOPTS_VALUE;
>>> @@ -783,10 +775,9 @@ end:
>>>            /* Add EOS tag */
>>>            for (i = 0; i < s->nb_streams; i++) {
>>>                AVCodecParameters *par = s->streams[i]->codecpar;
>>> -            FLVStreamContext *sc = s->streams[i]->priv_data;
>>>                if (par->codec_type == AVMEDIA_TYPE_VIDEO &&
>>>                        (par->codec_id == AV_CODEC_ID_H264 || par->codec_id == AV_CODEC_ID_MPEG4))
>>> -                put_eos_tag(pb, sc->last_ts, par->codec_id);
>>> +                put_eos_tag(pb, flv->last_ts[i], par->codec_id);
>>>            }
>>>        }
>>>    
>>> @@ -821,7 +812,6 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>        AVIOContext *pb      = s->pb;
>>>        AVCodecParameters *par = s->streams[pkt->stream_index]->codecpar;
>>>        FLVContext *flv      = s->priv_data;
>>> -    FLVStreamContext *sc = s->streams[pkt->stream_index]->priv_data;
>>>        unsigned ts;
>>>        int size = pkt->size;
>>>        uint8_t *data = NULL;
>>> @@ -919,13 +909,13 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>        }
>>>    
>>>        /* check Speex packet duration */
>>> -    if (par->codec_id == AV_CODEC_ID_SPEEX && ts - sc->last_ts > 160)
>>> +    if (par->codec_id == AV_CODEC_ID_SPEEX && ts - flv->last_ts[pkt->stream_index] > 160)
>>>            av_log(s, AV_LOG_WARNING, "Warning: Speex stream has more than "
>>>                                      "8 frames per packet. Adobe Flash "
>>>                                      "Player cannot handle this!\n");
>>>    
>>> -    if (sc->last_ts < ts)
>>> -        sc->last_ts = ts;
>>> +    if (flv->last_ts[pkt->stream_index] < ts)
>>> +        flv->last_ts[pkt->stream_index] = ts;
>>>    
>>>        if (size + flags_size >= 1<<24) {
>>>            av_log(s, AV_LOG_ERROR, "Too large packet with size %u >= %u\n",
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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".