| Message ID | 1469985744-30271-1-git-send-email-skw@google.com |
|---|---|
| State | Rejected |
| Headers | show |
Le quartidi 14 thermidor, an CCXXIV, Sophia Wang a écrit : > Since matroska->done is only set to 1 in matroska_resync(), the choice > of error is made by checking the return value of matroska_resync() > rather than checking matroska->done directly on the next > while-iteration. This is not what your code do: > + if (matroska_resync(matroska, pos) < 0) > + return avio_feof(s->pb) ? AVERROR_EOF : AVERROR(EIO); It checks the return value of matroska_resync() and then invents a completely unrelated error code. What it should do is use the error code from matroska_resync(). As simple as that. Regards,
Thanks for the quick response. matroska_resync() is currently written to only return AVERROR_EOF, so using that error code directly wouldn't be any more informative. But your suggestion would work if matroska_resync() kept the error code returned by avio_seek(). Would such a change warrant splitting into a separate patch? On Sun, Jul 31, 2016 at 10:26 AM, Nicolas George <george@nsup.org> wrote: > Le quartidi 14 thermidor, an CCXXIV, Sophia Wang a écrit : > > Since matroska->done is only set to 1 in matroska_resync(), the choice > > of error is made by checking the return value of matroska_resync() > > rather than checking matroska->done directly on the next > > while-iteration. > > This is not what your code do: > > > + if (matroska_resync(matroska, pos) < 0) > > + return avio_feof(s->pb) ? AVERROR_EOF : AVERROR(EIO); > > It checks the return value of matroska_resync() and then invents a > completely unrelated error code. > > What it should do is use the error code from matroska_resync(). As simple > as > that. > > Regards, > > -- > Nicolas George >
Le quintidi 15 thermidor, an CCXXIV, Sophia Wang a écrit : > Thanks for the quick response. matroska_resync() is currently written to > only return AVERROR_EOF, so using that error code directly wouldn't be any > more informative. But your suggestion would work if matroska_resync() kept > the error code returned by avio_seek(). Would such a change warrant > splitting into a separate patch? It is all the same issue of callers discarding the return code of called functions and then inventing a new one. It happens all over the place in this demuxer, and a recent commit by Carl Eugen shows it also happens in the rm demuxer. I think is is a remnant of the time before the AVERROR codes. You are of course not required to fix all the instances of the issue, especially when testing is not convenient, but I think fixing any number of them can belong in the same patch, especially if they all serve to fix a particular visible instance. Note that I am not maintainer of the Matroska code, though. Regards,
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index f3d701f..ed28a90 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -3322,10 +3322,10 @@ static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt) 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 (matroska_resync(matroska, pos) < 0) + return avio_feof(s->pb) ? AVERROR_EOF : AVERROR(EIO); + } } return 0;
Since matroska->done is only set to 1 in matroska_resync(), the choice of error is made by checking the return value of matroska_resync() rather than checking matroska->done directly on the next while-iteration. Signed-off-by: Sophia Wang <skw@google.com> --- libavformat/matroskadec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)