| Message ID | 1470849848-80918-1-git-send-email-skw@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Aug 10, 2016 at 10:24:08AM -0700, Sophia Wang wrote: > Signed-off-by: Sophia Wang <skw@google.com> > --- > libavformat/matroskadec.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index d07a092..8c809ad 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s); > static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos) > { > AVIOContext *pb = matroska->ctx->pb; > + int64_t ret; > uint32_t id; > matroska->current_id = 0; > matroska->num_levels = 0; > > /* seek to next position to resync from */ > - if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0) > - goto eof; > + if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) { > + matroska->done = 1; > + return ret; > + } > > id = avio_rb32(pb); > > @@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos) > id = (id << 8) | avio_r8(pb); > } > > -eof: > matroska->done = 1; > return AVERROR_EOF; > } > @@ -3322,13 +3324,16 @@ static int matroska_parse_cluster(MatroskaDemuxContext *matroska) > static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt) > { > MatroskaDemuxContext *matroska = s->priv_data; > + int ret; > > while (matroska_deliver_packet(matroska, pkt)) { > int64_t pos = avio_tell(matroska->ctx->pb); > if (matroska->done) > return AVERROR_EOF; > - if (matroska_parse_cluster(matroska) < 0) > - matroska_resync(matroska, pos); > + if (matroska_parse_cluster(matroska) < 0) { > + if ((ret = matroska_resync(matroska, pos)) < 0) > + return ret; > + } > } is it possible that matroska_parse_cluster() adds packets to matroska->packets and then fails? if so and if matroska_resync subsequently fails too the previous code would have returned the packet in matroska_deliver_packet() the new code would not i think unless i miss something (the application would likley not call matroska_read_packet() again after it signaled EOF, and might or might not after an error even though there would the still be buffered packets prior to the error [...]
Of the 3 functions that directly add to matroska->packets, 2 of them will always return 0 afterwards, and the third (matroska_parse_rm_audio) will only return something other than 0 in the case of AVERROR(ENOMEM) or AVERROR(EINVAL). All 3 functions are called from matroska_parse_block, which immediately returns the result if there is no error. If an error does occur, then the error code may eventually be returned, or it may be overwritten by the return value of any of the 3 packet-adding functions. matroska_parse_block is called from matroska_parse_cluster and its helper matroska_parse_cluster_incremental, both of which simply return the result (though it maybe overwritten by another invocation in matroska_parse_cluster). So in any case, if packets are added, the only sources of failure are ENOMEM and EINVAL, and I'm not sure if it makes sense to write code to work around those. On Wed, Aug 10, 2016 at 4:51 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Wed, Aug 10, 2016 at 10:24:08AM -0700, Sophia Wang wrote: > > Signed-off-by: Sophia Wang <skw@google.com> > > --- > > libavformat/matroskadec.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > > index d07a092..8c809ad 100644 > > --- a/libavformat/matroskadec.c > > +++ b/libavformat/matroskadec.c > > @@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext > *s); > > static int matroska_resync(MatroskaDemuxContext *matroska, int64_t > last_pos) > > { > > AVIOContext *pb = matroska->ctx->pb; > > + int64_t ret; > > uint32_t id; > > matroska->current_id = 0; > > matroska->num_levels = 0; > > > > /* seek to next position to resync from */ > > - if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0) > > - goto eof; > > + if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) { > > + matroska->done = 1; > > + return ret; > > + } > > > > id = avio_rb32(pb); > > > > @@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext > *matroska, int64_t last_pos) > > id = (id << 8) | avio_r8(pb); > > } > > > > -eof: > > matroska->done = 1; > > return AVERROR_EOF; > > } > > > @@ -3322,13 +3324,16 @@ static int matroska_parse_cluster(MatroskaDemuxContext > *matroska) > > static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt) > > { > > MatroskaDemuxContext *matroska = s->priv_data; > > + int ret; > > > > while (matroska_deliver_packet(matroska, pkt)) { > > int64_t pos = avio_tell(matroska->ctx->pb); > > if (matroska->done) > > return AVERROR_EOF; > > - if (matroska_parse_cluster(matroska) < 0) > > - matroska_resync(matroska, pos); > > + if (matroska_parse_cluster(matroska) < 0) { > > + if ((ret = matroska_resync(matroska, pos)) < 0) > > + return ret; > > + } > > } > > is it possible that matroska_parse_cluster() adds packets to > matroska->packets and then fails? > if so and if matroska_resync subsequently fails too > the previous code would have returned the packet in > matroska_deliver_packet() the new code would not i think unless > i miss something > (the application would likley not call matroska_read_packet() again > after it signaled EOF, and might or might not after an error even > though there would the still be buffered packets prior to the error > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > No great genius has ever existed without some touch of madness. -- > Aristotle > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
On Mon, Aug 15, 2016 at 10:27:47AM -0700, Sophia Wang wrote: > Of the 3 functions that directly add to matroska->packets, 2 of them will > always return 0 afterwards, and the third (matroska_parse_rm_audio) will > only return something other than 0 in the case of AVERROR(ENOMEM) or > AVERROR(EINVAL). but they can add multiple packets, so even if they return 0 there could be packets in the buffer after one is consumed > All 3 functions are called from matroska_parse_block, > which immediately returns the result if there is no error. did you mean "if there is AN error" ? > If an error does > occur, then the error code may eventually be returned, or it may be > overwritten by the return value of any of the 3 packet-adding functions. > > matroska_parse_block is called from matroska_parse_cluster and its helper > matroska_parse_cluster_incremental, both of which simply return the result > (though it maybe overwritten by another invocation in > matroska_parse_cluster). So in any case, if packets are added, the only > sources of failure are ENOMEM and EINVAL, and I'm not sure if it makes > sense to write code to work around those. you add code in matroska_resync() and its caller to immedeatly break out and return failure if the seek fails. At this point there can still be packets in the buffers unless i miss something, should the error not be returned after the packets that where read before the error ? [...]
I think you're right. Seems like the best solution is to maintain the number of calls to matroska_deliver_packet(); patch modification on the way. On Wed, Aug 31, 2016 at 6:30 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > > On Mon, Aug 15, 2016 at 10:27:47AM -0700, Sophia Wang wrote: > > Of the 3 functions that directly add to matroska->packets, 2 of them will > > always return 0 afterwards, and the third (matroska_parse_rm_audio) will > > only return something other than 0 in the case of AVERROR(ENOMEM) or > > AVERROR(EINVAL). > > but they can add multiple packets, so even if they return 0 there > could be packets in the buffer after one is consumed > > > > All 3 functions are called from matroska_parse_block, > > which immediately returns the result if there is no error. > > did you mean "if there is AN error" ? > > > > If an error does > > occur, then the error code may eventually be returned, or it may be > > overwritten by the return value of any of the 3 packet-adding functions. > > > > matroska_parse_block is called from matroska_parse_cluster and its helper > > matroska_parse_cluster_incremental, both of which simply return the > result > > (though it maybe overwritten by another invocation in > > matroska_parse_cluster). So in any case, if packets are added, the only > > sources of failure are ENOMEM and EINVAL, and I'm not sure if it makes > > sense to write code to work around those. > > you add code in matroska_resync() and its caller to immedeatly > break out and return failure if the seek fails. > At this point there can still be packets in the buffers unless i > miss something, should the error not be returned after the packets > that where read before the error ? > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > No great genius has ever existed without some touch of madness. -- > Aristotle > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index d07a092..8c809ad 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s); static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos) { AVIOContext *pb = matroska->ctx->pb; + int64_t ret; uint32_t id; matroska->current_id = 0; matroska->num_levels = 0; /* seek to next position to resync from */ - if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0) - goto eof; + if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) { + matroska->done = 1; + return ret; + } id = avio_rb32(pb); @@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos) id = (id << 8) | avio_r8(pb); } -eof: matroska->done = 1; return AVERROR_EOF; } @@ -3322,13 +3324,16 @@ static int matroska_parse_cluster(MatroskaDemuxContext *matroska) static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt) { MatroskaDemuxContext *matroska = s->priv_data; + int ret; while (matroska_deliver_packet(matroska, pkt)) { int64_t pos = avio_tell(matroska->ctx->pb); if (matroska->done) return AVERROR_EOF; - if (matroska_parse_cluster(matroska) < 0) - matroska_resync(matroska, pos); + if (matroska_parse_cluster(matroska) < 0) { + if ((ret = matroska_resync(matroska, pos)) < 0) + return ret; + } } return 0;
Signed-off-by: Sophia Wang <skw@google.com> --- libavformat/matroskadec.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)