[FFmpeg-devel] [PATCH 14/21] avformat/matroskadec: Use proper levels after discontínuity
Steve Lhomme
robux4 at ycbcr.xyz
Sun Apr 7 10:30:51 EEST 2019
On 3/27/2019 12:18 PM, Andreas Rheinhardt via ffmpeg-devel wrote:
> The earlier code set the level to zero upon seeking and after a
> discontinuity although in both cases parsing (re)starts at a level 1
> element.
>
> Also set the segment's length to unkown if an error occured in order not
> to drop any valid data that happens to be beyond the designated end of
> the segment.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at googlemail.com>
> ---
> libavformat/matroskadec.c | 59 +++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 0179e5426e..42f1c21042 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -737,13 +737,24 @@ static const char *const matroska_doctypes[] = { "matroska", "webm" };
>
> static int matroska_read_close(AVFormatContext *s);
>
> +static int matroska_reset_status(MatroskaDemuxContext *matroska,
> + uint32_t id, int64_t position)
> +{
> + matroska->current_id = id;
> + matroska->num_levels = 1;
> + matroska->current_cluster.pos = 0;
> +
> + if (position >= 0)
> + return avio_seek(matroska->ctx->pb, position, SEEK_SET);
> +
> + return 0;
> +}
> +
I think you should have done this commit in 2 parts.
- adding matroska_reset_status() and do exactly what was done before
- add changes related to the level and unknown length/discontinuity.
> 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 ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
> @@ -759,7 +770,14 @@ static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
> id == MATROSKA_ID_CUES || id == MATROSKA_ID_TAGS ||
> id == MATROSKA_ID_SEEKHEAD || id == MATROSKA_ID_ATTACHMENTS ||
> id == MATROSKA_ID_CLUSTER || id == MATROSKA_ID_CHAPTERS) {
> - matroska->current_id = id;
> + /* Prepare the context for further parsing of a level 1 element. */
> + matroska_reset_status(matroska, id, -1);
You set num_levels to 1 now, leaving this function used to have
num_levels set to 0. I'm not sure it's correct or not.
> +
> + /* Given that we are here means that an error has occured,
I thought this function was meant to find a level1 ID after getting into
a Segment. This does not seem like an error at all.
> + * so treat the segment as unknown length in order not to
> + * discard valid data that happens to be beyond the designated
> + * end of the segment. */
> + matroska->levels[0].length = EBML_UNKNOWN_LENGTH;
> return 0;
> }
> id = (id << 8) | avio_r8(pb);
> @@ -1610,18 +1628,12 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
> matroska->current_id = 0;
>
> ret = ebml_parse(matroska, matroska_segment, matroska);
> -
> - /* remove dummy level */
> - while (matroska->num_levels) {
> - uint64_t length = matroska->levels[--matroska->num_levels].length;
> - if (length == EBML_UNKNOWN_LENGTH)
> - break;
> - }
I think this code indicates unknown length was handled in a seekhead
entry, probably because such files exist. Making the assumption in 13/21
about unknown length only on Segment+Cluster incorrect.
> }
> }
> - /* seek back */
> - avio_seek(matroska->ctx->pb, before_pos, SEEK_SET);
> - matroska->current_id = saved_id;
> +
> + /* Seek back - notice that in all instances where this is used it is safe
> + * to set the level to 1 and unset the position of the current cluster. */
> + matroska_reset_status(matroska, saved_id, before_pos);
>
> return ret;
> }
> @@ -3535,9 +3547,7 @@ static int matroska_read_seek(AVFormatContext *s, int stream_index,
> timestamp = FFMAX(timestamp, st->index_entries[0].timestamp);
>
> if ((index = av_index_search_timestamp(st, timestamp, flags)) < 0 || index == st->nb_index_entries - 1) {
> - avio_seek(s->pb, st->index_entries[st->nb_index_entries - 1].pos,
> - SEEK_SET);
> - matroska->current_id = 0;
> + matroska_reset_status(matroska, 0, st->index_entries[st->nb_index_entries - 1].pos);
> while ((index = av_index_search_timestamp(st, timestamp, flags)) < 0 || index == st->nb_index_entries - 1) {
> matroska_clear_queue(matroska);
> if (matroska_parse_cluster(matroska) < 0)
> @@ -3557,8 +3567,8 @@ static int matroska_read_seek(AVFormatContext *s, int stream_index,
> tracks[i].end_timecode = 0;
> }
>
> - avio_seek(s->pb, st->index_entries[index].pos, SEEK_SET);
> - matroska->current_id = 0;
> + /* We seek to a level 1 element, so set the appropriate status. */
> + matroska_reset_status(matroska, 0, st->index_entries[index].pos);
> if (flags & AVSEEK_FLAG_ANY) {
> st->skip_to_keyframe = 0;
> matroska->skip_to_timecode = timestamp;
> @@ -3568,18 +3578,16 @@ static int matroska_read_seek(AVFormatContext *s, int stream_index,
> }
> matroska->skip_to_keyframe = 1;
> matroska->done = 0;
> - matroska->num_levels = 0;
> ff_update_cur_dts(s, st, st->index_entries[index].timestamp);
> return 0;
> err:
> // slightly hackish but allows proper fallback to
> // the generic seeking code.
> + matroska_reset_status(matroska, 0, -1);
> matroska_clear_queue(matroska);
> - matroska->current_id = 0;
> st->skip_to_keyframe =
> matroska->skip_to_keyframe = 0;
> matroska->done = 0;
> - matroska->num_levels = 0;
> return -1;
> }
>
> @@ -3662,8 +3670,8 @@ static int webm_clusters_start_with_keyframe(AVFormatContext *s)
> read = ebml_read_length(matroska, matroska->ctx->pb, &cluster_length);
> if (read < 0)
> break;
> - avio_seek(s->pb, cluster_pos, SEEK_SET);
> - matroska->current_id = 0;
> +
> + matroska_reset_status(matroska, 0, cluster_pos);
> matroska_clear_queue(matroska);
> if (matroska_parse_cluster(matroska) < 0 ||
> !matroska->queue) {
> @@ -3677,7 +3685,10 @@ static int webm_clusters_start_with_keyframe(AVFormatContext *s)
> break;
> }
> }
> - avio_seek(s->pb, before_pos, SEEK_SET);
> +
> + /* Restore the status after matroska_read_header: */
> + matroska_reset_status(matroska, MATROSKA_ID_CLUSTER, before_pos);
> +
> return rv;
> }
>
> --
> 2.19.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list