Re: [FFmpeg-devel] [PATCH v4 2/3] avformat/flvenc: support enhanced flv PacketTypeMetadata

[zhupengfei via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

> 2024年3月2日 22:59，Andreas Rheinhardt <andreas.rheinhardt@outlook.com> 写道：
> 
> zhupengfei via ffmpeg-devel:
>> From: Zhu Pengfei <411294962@qq.com>
>> 
>> Signed-off-by: Zhu Pengfei <411294962@qq.com>
>> ---
>> libavformat/flvenc.c | 142 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 142 insertions(+)
>> 
>> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
>> index f6d10f331c..ae7ea84bad 100644
>> --- a/libavformat/flvenc.c
>> +++ b/libavformat/flvenc.c
>> @@ -24,6 +24,7 @@
>> #include "libavutil/intfloat.h"
>> #include "libavutil/avassert.h"
>> #include "libavutil/mathematics.h"
>> +#include "libavutil/mastering_display_metadata.h"
>> #include "libavcodec/codec_desc.h"
>> #include "libavcodec/mpeg4audio.h"
>> #include "avio.h"
>> @@ -124,6 +125,7 @@ typedef struct FLVContext {
>> 
>>     int flags;
>>     int64_t last_ts[FLV_STREAM_TYPE_NB];
>> +    int write_metadata_pkt;
> 
> Should be metadata_pkt_written to be consistent with its semantics.
> 
>> } FLVContext;
>> 
>> static int get_audio_flags(AVFormatContext *s, AVCodecParameters *par)
>> @@ -478,6 +480,145 @@ static void write_metadata(AVFormatContext *s, unsigned int ts)
>>     avio_wb32(pb, flv->metadata_totalsize + 11);
>> }
>> 
>> +static void flv_write_metadata_packet(AVFormatContext *s, AVCodecParameters *par, unsigned int ts)
>> +{
>> +    AVIOContext *pb = s->pb;
>> +    FLVContext *flv = s->priv_data;
>> +    AVContentLightMetadata *lightMetadata = NULL;
>> +    AVMasteringDisplayMetadata *displayMetadata = NULL;
> 
> Can be constified.
> 
>> +    const int flags_size = 5;
>> +    int64_t metadata_size_pos = 0;
>> +    int64_t total_size = 0;
>> +    const AVPacketSideData *side_data = NULL;
>> +
>> +    if (par->codec_id == AV_CODEC_ID_HEVC || par->codec_id == AV_CODEC_ID_AV1 ||
>> +        par->codec_id == AV_CODEC_ID_VP9) {
> 
> You can avoid one level of indentation by returning early here.
> 
>> +        if (flv->write_metadata_pkt) return;
>> +
>> +        side_data = av_packet_side_data_get(par->coded_side_data, par->nb_coded_side_data,
>> +                                            AV_PKT_DATA_CONTENT_LIGHT_LEVEL);
>> +        if (side_data)
>> +            lightMetadata = (AVContentLightMetadata *)side_data->data;
>> +
>> +        side_data = av_packet_side_data_get(par->coded_side_data, par->nb_coded_side_data,
>> +                                            AV_PKT_DATA_MASTERING_DISPLAY_METADATA);
>> +        if (side_data)
>> +            displayMetadata = (AVMasteringDisplayMetadata *)side_data->data;
>> +
>> +        /*
>> +        * Reference Enhancing FLV
>> +        * https://github.com/veovera/enhanced-rtmp/blob/main/enhanced-rtmp.pdf
>> +        * */
>> +        avio_w8(pb, FLV_TAG_TYPE_VIDEO); //write video tag type
>> +        metadata_size_pos = avio_tell(pb);
>> +        avio_wb24(pb, 0 + flags_size);
>> +        put_timestamp(pb, ts); //ts = pkt->dts, gen
>> +        avio_wb24(pb, flv->reserved);
>> +
>> +        if (par->codec_id == AV_CODEC_ID_HEVC) {
>> +            avio_w8(pb, FLV_IS_EX_HEADER | PacketTypeMetadata| FLV_FRAME_VIDEO_INFO_CMD); // ExVideoTagHeader mode with PacketTypeMetadata
>> +            avio_write(pb, "hvc1", 4);
>> +        } else if (par->codec_id == AV_CODEC_ID_AV1 || par->codec_id == AV_CODEC_ID_VP9) {
>> +            avio_w8(pb, FLV_IS_EX_HEADER | PacketTypeMetadata| FLV_FRAME_VIDEO_INFO_CMD);
>> +            avio_write(pb, par->codec_id == AV_CODEC_ID_AV1 ? "av01" : "vp09", 4);
>> +        }
>> +
>> +        avio_w8(pb, AMF_DATA_TYPE_STRING);
>> +        put_amf_string(pb, "colorInfo");
>> +
>> +        avio_w8(pb, AMF_DATA_TYPE_OBJECT);
>> +
>> +        put_amf_string(pb, "colorConfig");  // colorConfig
>> +
>> +        avio_w8(pb, AMF_DATA_TYPE_OBJECT);
>> +
>> +        if (par->color_trc != AVCOL_TRC_UNSPECIFIED &&
>> +            par->color_trc < AVCOL_TRC_NB) {
>> +            put_amf_string(pb, "transferCharacteristics");  // color_trc
>> +            put_amf_double(pb, par->color_trc);
>> +        }
>> +
>> +        if (par->color_space != AVCOL_SPC_UNSPECIFIED &&
>> +            par->color_space < AVCOL_SPC_NB) {
>> +            put_amf_string(pb, "matrixCoefficients"); // colorspace
>> +            put_amf_double(pb, par->color_space);
>> +        }
>> +
>> +        if (par->color_primaries != AVCOL_PRI_UNSPECIFIED &&
>> +            par->color_primaries < AVCOL_PRI_NB) {
>> +            put_amf_string(pb, "colorPrimaries"); // color_primaries
>> +            put_amf_double(pb, par->color_primaries);
>> +        }
>> +
>> +        put_amf_string(pb, "");
>> +        avio_w8(pb, AMF_END_OF_OBJECT);
>> +
>> +        if (lightMetadata) {
>> +            put_amf_string(pb, "hdrCll");
>> +            avio_w8(pb, AMF_DATA_TYPE_OBJECT);
>> +
>> +            put_amf_string(pb, "maxFall");
>> +            put_amf_double(pb, lightMetadata->MaxFALL);
>> +
>> +            put_amf_string(pb, "maxCLL");
>> +            put_amf_double(pb, lightMetadata->MaxCLL);
>> +
>> +            put_amf_string(pb, "");
>> +            avio_w8(pb, AMF_END_OF_OBJECT);
>> +        }
>> +
>> +        if (displayMetadata && (displayMetadata->has_primaries || displayMetadata->has_luminance)) {
>> +            put_amf_string(pb, "hdrMdcv");
>> +            avio_w8(pb, AMF_DATA_TYPE_OBJECT);
>> +            if (displayMetadata->has_primaries) {
>> +                put_amf_string(pb, "redX");
>> +                put_amf_double(pb, av_q2d(displayMetadata->display_primaries[0][0]));
>> +
>> +                put_amf_string(pb, "redY");
>> +                put_amf_double(pb, av_q2d(displayMetadata->display_primaries[0][1]));
>> +
>> +                put_amf_string(pb, "greenX");
>> +                put_amf_double(pb, av_q2d(displayMetadata->display_primaries[1][0]));
>> +
>> +                put_amf_string(pb, "greenY");
>> +                put_amf_double(pb, av_q2d(displayMetadata->display_primaries[1][1]));
>> +
>> +                put_amf_string(pb, "blueX");
>> +                put_amf_double(pb, av_q2d(displayMetadata->display_primaries[2][0]));
>> +
>> +                put_amf_string(pb, "blueY");
>> +                put_amf_double(pb, av_q2d(displayMetadata->display_primaries[2][1]));
>> +
>> +                put_amf_string(pb, "whitePointX");
>> +                put_amf_double(pb, av_q2d(displayMetadata->white_point[0]));
>> +
>> +                put_amf_string(pb, "whitePointY");
>> +                put_amf_double(pb, av_q2d(displayMetadata->white_point[1]));
>> +            }
>> +            if (displayMetadata->has_luminance) {
>> +                put_amf_string(pb, "maxLuminance");
>> +                put_amf_double(pb, av_q2d(displayMetadata->max_luminance));
>> +
>> +                put_amf_string(pb, "minLuminance");
>> +                put_amf_double(pb, av_q2d(displayMetadata->min_luminance));
>> +            }
>> +            put_amf_string(pb, "");
>> +            avio_w8(pb, AMF_END_OF_OBJECT);
>> +        }
>> +        put_amf_string(pb, "");
>> +        avio_w8(pb, AMF_END_OF_OBJECT);
>> +
>> +        total_size = avio_tell(pb) - metadata_size_pos - 10;
>> +        avio_seek(pb, metadata_size_pos, SEEK_SET);
> 
> I see nothing that guarantees that this seek will succeed. In fact, this
> whole muxer seems to presume that the output is seekable, without
> checking so. Even worse: flv_write_header() clears the AVIOContext's
> seekability field in case the FLV_NO_METADATA flag is set.
> 

Hi Andreas: Thank you very much for your suggestion. The other comments have been modified.

1. "I see nothing that guarantees that this seek will succeed" 
	As you mentioned, most of the other muxer scenarios where avio_seek is used have no guarantees. In my thougth, it is relatively safe to use SEEK_SET to find the corresponding offset in this scenario.
2. "flv_write_header() clears the AVIOContext's seekability field in case the FLV_NO_METADATA flag is set"
	I checked the corresponding commit log and it was submitted by Steven Liu, which looks more like an expected feature.
    	"avformat/flvenc: add no_metadata to flvflags some flv have no metadata, ffmpeg will same with the source flv stream."


>> +        avio_wb24(pb, total_size);
>> +        avio_tell(pb);
>> +        avio_skip(pb, total_size + 10 - 3);
>> +        avio_tell(pb);
> 
> These two avio_tell() are useless.
> 
>> +        avio_wb32(pb, total_size + 11); // previous tag size
>> +        flv->write_metadata_pkt = 1;
>> +    }
>> +}
>> +
>> static int unsupported_codec(AVFormatContext *s,
>>                              const char* type, int codec_id)
>> {
>> @@ -878,6 +1019,7 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
>>             memcpy(par->extradata, side, side_size);
>>             flv_write_codec_header(s, par, pkt->dts);
>>         }
>> +        flv_write_metadata_packet(s, par, pkt->dts);
>>     }
>> 
>>     if (flv->delay == AV_NOPTS_VALUE)
> 
> _______________________________________________
> 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".