| Message ID | 20180511014008.1676-2-alex.converse@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 10, 2018 at 06:40:08PM -0700, Alex Converse wrote: > From: Alex Converse <alexconv@twitch.tv> > > --- > libavformat/flvenc.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) LGTM thx [...]
On Fri, May 11, 2018 at 4:40 AM, Alex Converse <alex.converse@gmail.com> wrote: > From: Alex Converse <alexconv@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
On Sun, May 13, 2018 at 12:25 PM, Jan Ekström <jeebjp@gmail.com> wrote: > On Fri, May 11, 2018 at 4:40 AM, Alex Converse <alex.converse@gmail.com> wrote: >> From: Alex Converse <alexconv@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
On Thu, May 10, 2018 at 06:40:08PM -0700, Alex Converse wrote: > From: Alex Converse <alexconv@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) { It seems jeeb prefers int64_t here instead of unsigned. Can you change that before pushing ? Thanks [...]
On Sun, May 13, 2018 at 3:24 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, May 10, 2018 at 06:40:08PM -0700, Alex Converse wrote: >> From: Alex Converse <alexconv@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) { > > It seems jeeb prefers int64_t here instead of unsigned. > Can you change that before pushing ? > Pushed with requested changes.
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); } }
From: Alex Converse <alexconv@twitch.tv> --- libavformat/flvenc.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)