| Message ID | 20191008054116.60992-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | ffb32d35eee616f79a37c4c96f37f2697932cc32 |
| Headers | show |
lgtm On 10/8/19, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Forgotten in 7da57875. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/mpeg.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c > index 6f132aae05..bd182e4429 100644 > --- a/libavformat/mpeg.c > +++ b/libavformat/mpeg.c > @@ -489,7 +489,6 @@ static int mpegps_read_packet(AVFormatContext *s, > MpegDemuxContext *m = s->priv_data; > AVStream *st; > int len, startcode, i, es_type, ret; > - int lpcm_header_len = -1; //Init to suppress warning > int pcm_dvd = 0; > int request_probe= 0; > enum AVCodecID codec_id = AV_CODEC_ID_NONE; > @@ -507,8 +506,7 @@ redo: > > if (!m->raw_ac3) { > /* audio: skip header */ > - avio_r8(s->pb); > - lpcm_header_len = avio_rb16(s->pb); > + avio_skip(s->pb, 3); > len -= 3; > if (startcode >= 0xb0 && startcode <= 0xbf) { > /* MLP/TrueHD audio has a 4-byte header */ > -- > 2.21.0 > > _______________________________________________ > 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".
Andreas Rheinhardt: > When vobsub_read_packet() reads a packet, it uses a dedicated AVPacket > to get the subtitle timing and position from an FFDemuxSubtitlesQueue > (which has been filled with this data during reading the idx file in > vobsub_read_header); afterwards the actual subtitle data is read into > the packet destined for output and the timing and position are copied > to this packet. Afterwards, the local packet is unreferenced. > > This can be simplified: Simply use the output packet to get the timing > and position from the FFDemuxSubtitlesQueue. The packet's size will be > zero afterwards, so that it can be directly used to read the actual > subtitle data. This makes copying the packet fields as well as > unreferencing the local packet unecessary and also removes an instance > of usage of sizeof(AVPacket) in libavformat. > > The only difference is that the returned packet will already be flagged > as a keyframe. This currently only happens in compute_pkt_fields(). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/mpeg.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) > > diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c > index bd182e4429..7daa72f7ce 100644 > --- a/libavformat/mpeg.c > +++ b/libavformat/mpeg.c > @@ -912,7 +912,6 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) > FFDemuxSubtitlesQueue *q; > AVIOContext *pb = vobsub->sub_ctx->pb; > int ret, psize, total_read = 0, i; > - AVPacket idx_pkt = { 0 }; > > int64_t min_ts = INT64_MAX; > int sid = 0; > @@ -927,24 +926,22 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) > } > } > q = &vobsub->q[sid]; > - ret = ff_subtitles_queue_read_packet(q, &idx_pkt); > + /* The returned packet will have size zero, > + * so that it can be directly used with av_grow_packet. */ > + ret = ff_subtitles_queue_read_packet(q, pkt); > if (ret < 0) > return ret; > > /* compute maximum packet size using the next packet position. This is > * useful when the len in the header is non-sense */ > if (q->current_sub_idx < q->nb_subs) { > - psize = q->subs[q->current_sub_idx].pos - idx_pkt.pos; > + psize = q->subs[q->current_sub_idx].pos - pkt->pos; > } else { > int64_t fsize = avio_size(pb); > - psize = fsize < 0 ? 0xffff : fsize - idx_pkt.pos; > + psize = fsize < 0 ? 0xffff : fsize - pkt->pos; > } > > - avio_seek(pb, idx_pkt.pos, SEEK_SET); > - > - av_init_packet(pkt); > - pkt->size = 0; > - pkt->data = NULL; > + avio_seek(pb, pkt->pos, SEEK_SET); > > do { > int n, to_read, startcode; > @@ -968,7 +965,7 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) > total_read += pkt_size; > > /* the current chunk doesn't match the stream index (unlikely) */ > - if ((startcode & 0x1f) != s->streams[idx_pkt.stream_index]->id) > + if ((startcode & 0x1f) != s->streams[pkt->stream_index]->id) > break; > > ret = av_grow_packet(pkt, to_read); > @@ -980,16 +977,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) > pkt->size -= to_read - n; > } while (total_read < psize); > > - pkt->pts = pkt->dts = idx_pkt.pts; > - pkt->pos = idx_pkt.pos; > - pkt->stream_index = idx_pkt.stream_index; > - > - av_packet_unref(&idx_pkt); > return 0; > > fail: > av_packet_unref(pkt); > - av_packet_unref(&idx_pkt); > return ret; > } > > Ping. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> When vobsub_read_packet() reads a packet, it uses a dedicated AVPacket >> to get the subtitle timing and position from an FFDemuxSubtitlesQueue >> (which has been filled with this data during reading the idx file in >> vobsub_read_header); afterwards the actual subtitle data is read into >> the packet destined for output and the timing and position are copied >> to this packet. Afterwards, the local packet is unreferenced. >> >> This can be simplified: Simply use the output packet to get the timing >> and position from the FFDemuxSubtitlesQueue. The packet's size will be >> zero afterwards, so that it can be directly used to read the actual >> subtitle data. This makes copying the packet fields as well as >> unreferencing the local packet unecessary and also removes an instance >> of usage of sizeof(AVPacket) in libavformat. >> >> The only difference is that the returned packet will already be flagged >> as a keyframe. This currently only happens in compute_pkt_fields(). >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/mpeg.c | 23 +++++++---------------- >> 1 file changed, 7 insertions(+), 16 deletions(-) >> >> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >> index bd182e4429..7daa72f7ce 100644 >> --- a/libavformat/mpeg.c >> +++ b/libavformat/mpeg.c >> @@ -912,7 +912,6 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >> FFDemuxSubtitlesQueue *q; >> AVIOContext *pb = vobsub->sub_ctx->pb; >> int ret, psize, total_read = 0, i; >> - AVPacket idx_pkt = { 0 }; >> >> int64_t min_ts = INT64_MAX; >> int sid = 0; >> @@ -927,24 +926,22 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >> } >> } >> q = &vobsub->q[sid]; >> - ret = ff_subtitles_queue_read_packet(q, &idx_pkt); >> + /* The returned packet will have size zero, >> + * so that it can be directly used with av_grow_packet. */ >> + ret = ff_subtitles_queue_read_packet(q, pkt); >> if (ret < 0) >> return ret; >> >> /* compute maximum packet size using the next packet position. This is >> * useful when the len in the header is non-sense */ >> if (q->current_sub_idx < q->nb_subs) { >> - psize = q->subs[q->current_sub_idx].pos - idx_pkt.pos; >> + psize = q->subs[q->current_sub_idx].pos - pkt->pos; >> } else { >> int64_t fsize = avio_size(pb); >> - psize = fsize < 0 ? 0xffff : fsize - idx_pkt.pos; >> + psize = fsize < 0 ? 0xffff : fsize - pkt->pos; >> } >> >> - avio_seek(pb, idx_pkt.pos, SEEK_SET); >> - >> - av_init_packet(pkt); >> - pkt->size = 0; >> - pkt->data = NULL; >> + avio_seek(pb, pkt->pos, SEEK_SET); >> >> do { >> int n, to_read, startcode; >> @@ -968,7 +965,7 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >> total_read += pkt_size; >> >> /* the current chunk doesn't match the stream index (unlikely) */ >> - if ((startcode & 0x1f) != s->streams[idx_pkt.stream_index]->id) >> + if ((startcode & 0x1f) != s->streams[pkt->stream_index]->id) >> break; >> >> ret = av_grow_packet(pkt, to_read); >> @@ -980,16 +977,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >> pkt->size -= to_read - n; >> } while (total_read < psize); >> >> - pkt->pts = pkt->dts = idx_pkt.pts; >> - pkt->pos = idx_pkt.pos; >> - pkt->stream_index = idx_pkt.stream_index; >> - >> - av_packet_unref(&idx_pkt); >> return 0; >> >> fail: >> av_packet_unref(pkt); >> - av_packet_unref(&idx_pkt); >> return ret; >> } >> >> > Ping. > > - Andreas > Another ping for the three unmerged patches of this patchset. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Andreas Rheinhardt: >>> When vobsub_read_packet() reads a packet, it uses a dedicated AVPacket >>> to get the subtitle timing and position from an FFDemuxSubtitlesQueue >>> (which has been filled with this data during reading the idx file in >>> vobsub_read_header); afterwards the actual subtitle data is read into >>> the packet destined for output and the timing and position are copied >>> to this packet. Afterwards, the local packet is unreferenced. >>> >>> This can be simplified: Simply use the output packet to get the timing >>> and position from the FFDemuxSubtitlesQueue. The packet's size will be >>> zero afterwards, so that it can be directly used to read the actual >>> subtitle data. This makes copying the packet fields as well as >>> unreferencing the local packet unecessary and also removes an instance >>> of usage of sizeof(AVPacket) in libavformat. >>> >>> The only difference is that the returned packet will already be flagged >>> as a keyframe. This currently only happens in compute_pkt_fields(). >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavformat/mpeg.c | 23 +++++++---------------- >>> 1 file changed, 7 insertions(+), 16 deletions(-) >>> >>> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >>> index bd182e4429..7daa72f7ce 100644 >>> --- a/libavformat/mpeg.c >>> +++ b/libavformat/mpeg.c >>> @@ -912,7 +912,6 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>> FFDemuxSubtitlesQueue *q; >>> AVIOContext *pb = vobsub->sub_ctx->pb; >>> int ret, psize, total_read = 0, i; >>> - AVPacket idx_pkt = { 0 }; >>> >>> int64_t min_ts = INT64_MAX; >>> int sid = 0; >>> @@ -927,24 +926,22 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>> } >>> } >>> q = &vobsub->q[sid]; >>> - ret = ff_subtitles_queue_read_packet(q, &idx_pkt); >>> + /* The returned packet will have size zero, >>> + * so that it can be directly used with av_grow_packet. */ >>> + ret = ff_subtitles_queue_read_packet(q, pkt); >>> if (ret < 0) >>> return ret; >>> >>> /* compute maximum packet size using the next packet position. This is >>> * useful when the len in the header is non-sense */ >>> if (q->current_sub_idx < q->nb_subs) { >>> - psize = q->subs[q->current_sub_idx].pos - idx_pkt.pos; >>> + psize = q->subs[q->current_sub_idx].pos - pkt->pos; >>> } else { >>> int64_t fsize = avio_size(pb); >>> - psize = fsize < 0 ? 0xffff : fsize - idx_pkt.pos; >>> + psize = fsize < 0 ? 0xffff : fsize - pkt->pos; >>> } >>> >>> - avio_seek(pb, idx_pkt.pos, SEEK_SET); >>> - >>> - av_init_packet(pkt); >>> - pkt->size = 0; >>> - pkt->data = NULL; >>> + avio_seek(pb, pkt->pos, SEEK_SET); >>> >>> do { >>> int n, to_read, startcode; >>> @@ -968,7 +965,7 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>> total_read += pkt_size; >>> >>> /* the current chunk doesn't match the stream index (unlikely) */ >>> - if ((startcode & 0x1f) != s->streams[idx_pkt.stream_index]->id) >>> + if ((startcode & 0x1f) != s->streams[pkt->stream_index]->id) >>> break; >>> >>> ret = av_grow_packet(pkt, to_read); >>> @@ -980,16 +977,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>> pkt->size -= to_read - n; >>> } while (total_read < psize); >>> >>> - pkt->pts = pkt->dts = idx_pkt.pts; >>> - pkt->pos = idx_pkt.pos; >>> - pkt->stream_index = idx_pkt.stream_index; >>> - >>> - av_packet_unref(&idx_pkt); >>> return 0; >>> >>> fail: >>> av_packet_unref(pkt); >>> - av_packet_unref(&idx_pkt); >>> return ret; >>> } >>> >>> >> Ping. >> >> - Andreas >> > Another ping for the three unmerged patches of this patchset. > > - Andreas > Another ping for the two unmerged patches of this patchset; and also for the other patch fixing uninitialized reads in the vobsub demuxer: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-October/251961.html - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Andreas Rheinhardt: >>> Andreas Rheinhardt: >>>> When vobsub_read_packet() reads a packet, it uses a dedicated AVPacket >>>> to get the subtitle timing and position from an FFDemuxSubtitlesQueue >>>> (which has been filled with this data during reading the idx file in >>>> vobsub_read_header); afterwards the actual subtitle data is read into >>>> the packet destined for output and the timing and position are copied >>>> to this packet. Afterwards, the local packet is unreferenced. >>>> >>>> This can be simplified: Simply use the output packet to get the timing >>>> and position from the FFDemuxSubtitlesQueue. The packet's size will be >>>> zero afterwards, so that it can be directly used to read the actual >>>> subtitle data. This makes copying the packet fields as well as >>>> unreferencing the local packet unecessary and also removes an instance >>>> of usage of sizeof(AVPacket) in libavformat. >>>> >>>> The only difference is that the returned packet will already be flagged >>>> as a keyframe. This currently only happens in compute_pkt_fields(). >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> libavformat/mpeg.c | 23 +++++++---------------- >>>> 1 file changed, 7 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c >>>> index bd182e4429..7daa72f7ce 100644 >>>> --- a/libavformat/mpeg.c >>>> +++ b/libavformat/mpeg.c >>>> @@ -912,7 +912,6 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>>> FFDemuxSubtitlesQueue *q; >>>> AVIOContext *pb = vobsub->sub_ctx->pb; >>>> int ret, psize, total_read = 0, i; >>>> - AVPacket idx_pkt = { 0 }; >>>> >>>> int64_t min_ts = INT64_MAX; >>>> int sid = 0; >>>> @@ -927,24 +926,22 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>>> } >>>> } >>>> q = &vobsub->q[sid]; >>>> - ret = ff_subtitles_queue_read_packet(q, &idx_pkt); >>>> + /* The returned packet will have size zero, >>>> + * so that it can be directly used with av_grow_packet. */ >>>> + ret = ff_subtitles_queue_read_packet(q, pkt); >>>> if (ret < 0) >>>> return ret; >>>> >>>> /* compute maximum packet size using the next packet position. This is >>>> * useful when the len in the header is non-sense */ >>>> if (q->current_sub_idx < q->nb_subs) { >>>> - psize = q->subs[q->current_sub_idx].pos - idx_pkt.pos; >>>> + psize = q->subs[q->current_sub_idx].pos - pkt->pos; >>>> } else { >>>> int64_t fsize = avio_size(pb); >>>> - psize = fsize < 0 ? 0xffff : fsize - idx_pkt.pos; >>>> + psize = fsize < 0 ? 0xffff : fsize - pkt->pos; >>>> } >>>> >>>> - avio_seek(pb, idx_pkt.pos, SEEK_SET); >>>> - >>>> - av_init_packet(pkt); >>>> - pkt->size = 0; >>>> - pkt->data = NULL; >>>> + avio_seek(pb, pkt->pos, SEEK_SET); >>>> >>>> do { >>>> int n, to_read, startcode; >>>> @@ -968,7 +965,7 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>>> total_read += pkt_size; >>>> >>>> /* the current chunk doesn't match the stream index (unlikely) */ >>>> - if ((startcode & 0x1f) != s->streams[idx_pkt.stream_index]->id) >>>> + if ((startcode & 0x1f) != s->streams[pkt->stream_index]->id) >>>> break; >>>> >>>> ret = av_grow_packet(pkt, to_read); >>>> @@ -980,16 +977,10 @@ static int vobsub_read_packet(AVFormatContext *s, AVPacket *pkt) >>>> pkt->size -= to_read - n; >>>> } while (total_read < psize); >>>> >>>> - pkt->pts = pkt->dts = idx_pkt.pts; >>>> - pkt->pos = idx_pkt.pos; >>>> - pkt->stream_index = idx_pkt.stream_index; >>>> - >>>> - av_packet_unref(&idx_pkt); >>>> return 0; >>>> >>>> fail: >>>> av_packet_unref(pkt); >>>> - av_packet_unref(&idx_pkt); >>>> return ret; >>>> } >>>> >>>> >>> Ping. >>> >>> - Andreas >>> >> Another ping for the three unmerged patches of this patchset. >> >> - Andreas >> > Another ping for the two unmerged patches of this patchset; and also > for the other patch fixing uninitialized reads in the vobsub demuxer: > https://ffmpeg.org/pipermail/ffmpeg-devel/2019-October/251961.html > > - Andreas > Ping. - Andreas
On Tue, Oct 08, 2019 at 07:41:14AM +0200, Andreas Rheinhardt wrote: > When vobsub_read_packet() reads a packet, it uses a dedicated AVPacket > to get the subtitle timing and position from an FFDemuxSubtitlesQueue > (which has been filled with this data during reading the idx file in > vobsub_read_header); afterwards the actual subtitle data is read into > the packet destined for output and the timing and position are copied > to this packet. Afterwards, the local packet is unreferenced. > > This can be simplified: Simply use the output packet to get the timing > and position from the FFDemuxSubtitlesQueue. The packet's size will be > zero afterwards, so that it can be directly used to read the actual > subtitle data. This makes copying the packet fields as well as > unreferencing the local packet unecessary and also removes an instance > of usage of sizeof(AVPacket) in libavformat. > > The only difference is that the returned packet will already be flagged > as a keyframe. This currently only happens in compute_pkt_fields(). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/mpeg.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) will apply thx [...]
On Tue, Oct 8, 2019 at 7:42 AM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote: > If the size of the input packet is zero, av_grow_packet() used to call > av_new_packet() which would initialize the packet and (in particular) > reset the pos field. This behaviour (which was never documented and > arguably always contradicted the documented behaviour) was changed in > 2fe04630. This means that it is unnecessary to save and restore the > packet's position in append_packet_chunked(). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/utils.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 60f0229adc..64ec0b821a 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -268,7 +268,6 @@ int ffio_limit(AVIOContext *s, int size) > * Return the number of bytes read or an error. */ > static int append_packet_chunked(AVIOContext *s, AVPacket *pkt, int size) > { > - int64_t orig_pos = pkt->pos; // av_grow_packet might reset pos > int orig_size = pkt->size; > int ret; > > @@ -301,7 +300,6 @@ static int append_packet_chunked(AVIOContext *s, > AVPacket *pkt, int size) > if (size > 0) > pkt->flags |= AV_PKT_FLAG_CORRUPT; > > - pkt->pos = orig_pos; > if (!pkt->size) > av_packet_unref(pkt); > return pkt->size > orig_size ? pkt->size - orig_size : ret; > -- Ping. - Andreas
On Tue, Oct 08, 2019 at 07:41:15AM +0200, Andreas Rheinhardt wrote: > If the size of the input packet is zero, av_grow_packet() used to call > av_new_packet() which would initialize the packet and (in particular) > reset the pos field. This behaviour (which was never documented and > arguably always contradicted the documented behaviour) was changed in > 2fe04630. This means that it is unnecessary to save and restore the > packet's position in append_packet_chunked(). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/utils.c | 2 -- > 1 file changed, 2 deletions(-) will apply thx [...]
diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c index 6f132aae05..bd182e4429 100644 --- a/libavformat/mpeg.c +++ b/libavformat/mpeg.c @@ -489,7 +489,6 @@ static int mpegps_read_packet(AVFormatContext *s, MpegDemuxContext *m = s->priv_data; AVStream *st; int len, startcode, i, es_type, ret; - int lpcm_header_len = -1; //Init to suppress warning int pcm_dvd = 0; int request_probe= 0; enum AVCodecID codec_id = AV_CODEC_ID_NONE; @@ -507,8 +506,7 @@ redo: if (!m->raw_ac3) { /* audio: skip header */ - avio_r8(s->pb); - lpcm_header_len = avio_rb16(s->pb); + avio_skip(s->pb, 3); len -= 3; if (startcode >= 0xb0 && startcode <= 0xbf) { /* MLP/TrueHD audio has a 4-byte header */
Forgotten in 7da57875. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/mpeg.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)