On Sun, May 13, 2018 at 12:25 PM, Jan Ekström <jee...@gmail.com> wrote:
> On Fri, May 11, 2018 at 4:40 AM, Alex Converse <alex.conve...@gmail.com>
> wrote:
>> From: Alex Converse <alexc...@twitch.tv>
>>
>> ---
>> libavformat/flvenc.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
>> index 9b7cdfe7db..7aa2dbf9a6 100644
>> --- a/libavformat/flvenc.c
>> +++ b/libavformat/flvenc.c
>> @@ -485,7 +485,7 @@ static int unsupported_codec(AVFormatContext *s,
>> return AVERROR(ENOSYS);
>> }
>>
>> -static void flv_write_codec_header(AVFormatContext* s, AVCodecParameters*
>> par) {
>> +static void flv_write_codec_header(AVFormatContext* s, AVCodecParameters*
>> par, unsigned ts) {
>> int64_t data_size;
>> AVIOContext *pb = s->pb;
>> FLVContext *flv = s->priv_data;
>> @@ -497,8 +497,7 @@ static void flv_write_codec_header(AVFormatContext* s,
>> AVCodecParameters* par) {
>> par->codec_type == AVMEDIA_TYPE_VIDEO ?
>> FLV_TAG_TYPE_VIDEO : FLV_TAG_TYPE_AUDIO);
>> avio_wb24(pb, 0); // size patched later
>> - avio_wb24(pb, 0); // ts
>> - avio_w8(pb, 0); // ts ext
>> + put_timestamp(pb, ts);
>> avio_wb24(pb, 0); // streamid
>> pos = avio_tell(pb);
>> if (par->codec_id == AV_CODEC_ID_AAC) {
>> @@ -761,7 +760,7 @@ static int flv_write_header(AVFormatContext *s)
>> }
>>
>> for (i = 0; i < s->nb_streams; i++) {
>> - flv_write_codec_header(s, s->streams[i]->codecpar);
>> + flv_write_codec_header(s, s->streams[i]->codecpar, 0);
>> }
>>
>> flv->datastart_offset = avio_tell(pb);
>> @@ -905,7 +904,7 @@ static int flv_write_packet(AVFormatContext *s, AVPacket
>> *pkt)
>> }
>> memcpy(par->extradata, side, side_size);
>> par->extradata_size = side_size;
>> - flv_write_codec_header(s, par);
>> + flv_write_codec_header(s, par, (unsigned)pkt->dts);
>> }
>> }
>>
>
> Yes, this will get rid of the possible warning by casting, but you're
> still dealing with system specifically sized variables, so you might
> be doing int64_t ->uint64_t or int64_t->uint32_t.
>
> I have no idea why the aversion of just using the same type in
> `flv_write_codec_header` as what the DTS is (int64_t)? And why are we
> completely ignoring the fact that FLV has timestamp wrap-arounds? Or
> does this update header not care about that?
>
> Best regards,
> Jan
Hi,
OK, seemingly I'm the bad guy here and all such technical issues in
new code (including old modules) should just be ignored if it's just
copying stuff from the same place that is not fit for our coding
standards.
My original intention was to make sure that Alex's patch doesn't get
ignored and that it could have a nice review which could be handled in
a single or two iterations by looking into how FLV works and fixing up
some minor things like the int64_t->unsigned case. My idea was not to
fix other parts of flvenc, but just at the very least getting the
information on how to fix it fully afterwards if indeed some copied
code seemingly did things incorrectly, and just making this part as an
example of how to do that in the future.
And now it seems like these intentions went the wrong way, and most of
the people on IRC seem to say that since this same way was used in
flvenc before (int64_t->unsigned), it could be used again in newly
added code.
OK, I will not be a nuisance in this thread any more and any technical
notes I made here can be considered moot since I seem to have come out
as just being an asshole to Alex. I am sorry. That was never my
intention.
Best regards,
Jan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
