| Message ID | 20200321063118.6790-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/hnm: Check for extradata allocation failure | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
ok if fate passed and it is covered. On 3/21/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > The hnm demuxer's context struct contained lots of fields that are > write-only variables or that are not used outside of parsing the header > and that can therefore be replaced by local variables of hnm_read_header(). > This commit removes all of these from the context; the second type has > been replaced by local variables. > > An AVPacket (that was initialized when reading the header and for which > dead code to unreference it existed in hnm_read_close()) is among the > removed things. Removing it allowed to remove hnm_read_close() > altogether and also removes another instance of usage of sizeof(AVPacket). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/hnm.c | 63 +++++++++-------------------------------------- > 1 file changed, 12 insertions(+), 51 deletions(-) > > diff --git a/libavformat/hnm.c b/libavformat/hnm.c > index 31221553a4..f06add5cf8 100644 > --- a/libavformat/hnm.c > +++ b/libavformat/hnm.c > @@ -37,19 +37,9 @@ > #define HNM4_CHUNK_ID_SD 17491 > > typedef struct Hnm4DemuxContext { > - uint8_t version; > - uint16_t width; > - uint16_t height; > - uint32_t filesize; > uint32_t frames; > - uint32_t taboffset; > - uint16_t bits; > - uint16_t channels; > - uint32_t framesize; > uint32_t currentframe; > - int64_t pts; > uint32_t superchunk_remaining; > - AVPacket vpkt; > } Hnm4DemuxContext; > > static int hnm_probe(const AVProbeData *p) > @@ -69,55 +59,37 @@ static int hnm_read_header(AVFormatContext *s) > { > Hnm4DemuxContext *hnm = s->priv_data; > AVIOContext *pb = s->pb; > + unsigned width, height; > AVStream *vst; > int ret; > > - /* default context members */ > - hnm->pts = 0; > - av_init_packet(&hnm->vpkt); > - hnm->vpkt.data = NULL; > - hnm->vpkt.size = 0; > - > - hnm->superchunk_remaining = 0; > - > avio_skip(pb, 8); > - hnm->width = avio_rl16(pb); > - hnm->height = avio_rl16(pb); > - hnm->filesize = avio_rl32(pb); > + width = avio_rl16(pb); > + height = avio_rl16(pb); > + avio_rl32(pb); // filesize > hnm->frames = avio_rl32(pb); > - hnm->taboffset = avio_rl32(pb); > - hnm->bits = avio_rl16(pb); > - hnm->channels = avio_rl16(pb); > - hnm->framesize = avio_rl32(pb); > - avio_skip(pb, 32); > + avio_skip(pb, 44); > > - hnm->currentframe = 0; > - > - if (hnm->width < 256 || hnm->width > 640 || > - hnm->height < 150 || hnm->height > 480) { > + if (width < 256 || width > 640 || > + height < 150 || height > 480) { > av_log(s, AV_LOG_ERROR, > - "invalid resolution: %ux%u\n", hnm->width, hnm->height); > + "invalid resolution: %ux%u\n", width, height); > return AVERROR_INVALIDDATA; > } > > - // TODO: find a better way to detect HNM4A > - if (hnm->width == 640) > - hnm->version = 0x4a; > - else > - hnm->version = 0x40; > - > if (!(vst = avformat_new_stream(s, NULL))) > return AVERROR(ENOMEM); > > vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; > vst->codecpar->codec_id = AV_CODEC_ID_HNM4_VIDEO; > vst->codecpar->codec_tag = 0; > - vst->codecpar->width = hnm->width; > - vst->codecpar->height = hnm->height; > + vst->codecpar->width = width; > + vst->codecpar->height = height; > if ((ret = ff_alloc_extradata(vst->codecpar, 1)) < 0) > return ret; > > - vst->codecpar->extradata[0] = hnm->version; > + // TODO: find a better way to detect HNM4A > + vst->codecpar->extradata[0] = width == 640 ? 0x4a : 0x40; > > vst->start_time = 0; > > @@ -186,16 +158,6 @@ static int hnm_read_packet(AVFormatContext *s, AVPacket > *pkt) > return ret; > } > > -static int hnm_read_close(AVFormatContext *s) > -{ > - Hnm4DemuxContext *hnm = s->priv_data; > - > - if (hnm->vpkt.size > 0) > - av_packet_unref(&hnm->vpkt); > - > - return 0; > -} > - > AVInputFormat ff_hnm_demuxer = { > .name = "hnm", > .long_name = NULL_IF_CONFIG_SMALL("Cryo HNM v4"), > @@ -203,6 +165,5 @@ AVInputFormat ff_hnm_demuxer = { > .read_probe = hnm_probe, > .read_header = hnm_read_header, > .read_packet = hnm_read_packet, > - .read_close = hnm_read_close, > .flags = AVFMT_NO_BYTE_SEEK | AVFMT_NOGENSEARCH | > AVFMT_NOBINSEARCH > }; > -- > 2.20.1 > > _______________________________________________ > 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".
Paul B Mahol: > ok if fate passed and it is covered. > It is not covered, yet I checked all the samples from [1] as well as the one from ticket #3464. (That the stuff I removed is actually unused can also be seen from the fact that I did not modify hnm_read_packet() at all.) - Andreas [1]: https://samples.ffmpeg.org/game-formats/hnm/losteden-hnm4/
Paul B Mahol: > ok if fate passed and it is covered. > > On 3/21/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: >> The hnm demuxer's context struct contained lots of fields that are >> write-only variables or that are not used outside of parsing the header >> and that can therefore be replaced by local variables of hnm_read_header(). >> This commit removes all of these from the context; the second type has >> been replaced by local variables. >> >> An AVPacket (that was initialized when reading the header and for which >> dead code to unreference it existed in hnm_read_close()) is among the >> removed things. Removing it allowed to remove hnm_read_close() >> altogether and also removes another instance of usage of sizeof(AVPacket). >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/hnm.c | 63 +++++++++-------------------------------------- >> 1 file changed, 12 insertions(+), 51 deletions(-) >> >> diff --git a/libavformat/hnm.c b/libavformat/hnm.c >> index 31221553a4..f06add5cf8 100644 >> --- a/libavformat/hnm.c >> +++ b/libavformat/hnm.c >> @@ -37,19 +37,9 @@ >> #define HNM4_CHUNK_ID_SD 17491 >> >> typedef struct Hnm4DemuxContext { >> - uint8_t version; >> - uint16_t width; >> - uint16_t height; >> - uint32_t filesize; >> uint32_t frames; >> - uint32_t taboffset; >> - uint16_t bits; >> - uint16_t channels; >> - uint32_t framesize; >> uint32_t currentframe; >> - int64_t pts; >> uint32_t superchunk_remaining; >> - AVPacket vpkt; >> } Hnm4DemuxContext; >> >> static int hnm_probe(const AVProbeData *p) >> @@ -69,55 +59,37 @@ static int hnm_read_header(AVFormatContext *s) >> { >> Hnm4DemuxContext *hnm = s->priv_data; >> AVIOContext *pb = s->pb; >> + unsigned width, height; >> AVStream *vst; >> int ret; >> >> - /* default context members */ >> - hnm->pts = 0; >> - av_init_packet(&hnm->vpkt); >> - hnm->vpkt.data = NULL; >> - hnm->vpkt.size = 0; >> - >> - hnm->superchunk_remaining = 0; >> - >> avio_skip(pb, 8); >> - hnm->width = avio_rl16(pb); >> - hnm->height = avio_rl16(pb); >> - hnm->filesize = avio_rl32(pb); >> + width = avio_rl16(pb); >> + height = avio_rl16(pb); >> + avio_rl32(pb); // filesize >> hnm->frames = avio_rl32(pb); >> - hnm->taboffset = avio_rl32(pb); >> - hnm->bits = avio_rl16(pb); >> - hnm->channels = avio_rl16(pb); >> - hnm->framesize = avio_rl32(pb); >> - avio_skip(pb, 32); >> + avio_skip(pb, 44); >> >> - hnm->currentframe = 0; >> - >> - if (hnm->width < 256 || hnm->width > 640 || >> - hnm->height < 150 || hnm->height > 480) { >> + if (width < 256 || width > 640 || >> + height < 150 || height > 480) { >> av_log(s, AV_LOG_ERROR, >> - "invalid resolution: %ux%u\n", hnm->width, hnm->height); >> + "invalid resolution: %ux%u\n", width, height); >> return AVERROR_INVALIDDATA; >> } >> >> - // TODO: find a better way to detect HNM4A >> - if (hnm->width == 640) >> - hnm->version = 0x4a; >> - else >> - hnm->version = 0x40; >> - >> if (!(vst = avformat_new_stream(s, NULL))) >> return AVERROR(ENOMEM); >> >> vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; >> vst->codecpar->codec_id = AV_CODEC_ID_HNM4_VIDEO; >> vst->codecpar->codec_tag = 0; >> - vst->codecpar->width = hnm->width; >> - vst->codecpar->height = hnm->height; >> + vst->codecpar->width = width; >> + vst->codecpar->height = height; >> if ((ret = ff_alloc_extradata(vst->codecpar, 1)) < 0) >> return ret; >> >> - vst->codecpar->extradata[0] = hnm->version; >> + // TODO: find a better way to detect HNM4A >> + vst->codecpar->extradata[0] = width == 640 ? 0x4a : 0x40; >> >> vst->start_time = 0; >> >> @@ -186,16 +158,6 @@ static int hnm_read_packet(AVFormatContext *s, AVPacket >> *pkt) >> return ret; >> } >> >> -static int hnm_read_close(AVFormatContext *s) >> -{ >> - Hnm4DemuxContext *hnm = s->priv_data; >> - >> - if (hnm->vpkt.size > 0) >> - av_packet_unref(&hnm->vpkt); >> - >> - return 0; >> -} >> - >> AVInputFormat ff_hnm_demuxer = { >> .name = "hnm", >> .long_name = NULL_IF_CONFIG_SMALL("Cryo HNM v4"), >> @@ -203,6 +165,5 @@ AVInputFormat ff_hnm_demuxer = { >> .read_probe = hnm_probe, >> .read_header = hnm_read_header, >> .read_packet = hnm_read_packet, >> - .read_close = hnm_read_close, >> .flags = AVFMT_NO_BYTE_SEEK | AVFMT_NOGENSEARCH | >> AVFMT_NOBINSEARCH >> }; >> -- >> 2.20.1 Thanks, applied. - Andreas
diff --git a/libavformat/hnm.c b/libavformat/hnm.c index 31221553a4..f06add5cf8 100644 --- a/libavformat/hnm.c +++ b/libavformat/hnm.c @@ -37,19 +37,9 @@ #define HNM4_CHUNK_ID_SD 17491 typedef struct Hnm4DemuxContext { - uint8_t version; - uint16_t width; - uint16_t height; - uint32_t filesize; uint32_t frames; - uint32_t taboffset; - uint16_t bits; - uint16_t channels; - uint32_t framesize; uint32_t currentframe; - int64_t pts; uint32_t superchunk_remaining; - AVPacket vpkt; } Hnm4DemuxContext; static int hnm_probe(const AVProbeData *p) @@ -69,55 +59,37 @@ static int hnm_read_header(AVFormatContext *s) { Hnm4DemuxContext *hnm = s->priv_data; AVIOContext *pb = s->pb; + unsigned width, height; AVStream *vst; int ret; - /* default context members */ - hnm->pts = 0; - av_init_packet(&hnm->vpkt); - hnm->vpkt.data = NULL; - hnm->vpkt.size = 0; - - hnm->superchunk_remaining = 0; - avio_skip(pb, 8); - hnm->width = avio_rl16(pb); - hnm->height = avio_rl16(pb); - hnm->filesize = avio_rl32(pb); + width = avio_rl16(pb); + height = avio_rl16(pb); + avio_rl32(pb); // filesize hnm->frames = avio_rl32(pb); - hnm->taboffset = avio_rl32(pb); - hnm->bits = avio_rl16(pb); - hnm->channels = avio_rl16(pb); - hnm->framesize = avio_rl32(pb); - avio_skip(pb, 32); + avio_skip(pb, 44); - hnm->currentframe = 0; - - if (hnm->width < 256 || hnm->width > 640 || - hnm->height < 150 || hnm->height > 480) { + if (width < 256 || width > 640 || + height < 150 || height > 480) { av_log(s, AV_LOG_ERROR, - "invalid resolution: %ux%u\n", hnm->width, hnm->height); + "invalid resolution: %ux%u\n", width, height); return AVERROR_INVALIDDATA; } - // TODO: find a better way to detect HNM4A - if (hnm->width == 640) - hnm->version = 0x4a; - else - hnm->version = 0x40; - if (!(vst = avformat_new_stream(s, NULL))) return AVERROR(ENOMEM); vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; vst->codecpar->codec_id = AV_CODEC_ID_HNM4_VIDEO; vst->codecpar->codec_tag = 0; - vst->codecpar->width = hnm->width; - vst->codecpar->height = hnm->height; + vst->codecpar->width = width; + vst->codecpar->height = height; if ((ret = ff_alloc_extradata(vst->codecpar, 1)) < 0) return ret; - vst->codecpar->extradata[0] = hnm->version; + // TODO: find a better way to detect HNM4A + vst->codecpar->extradata[0] = width == 640 ? 0x4a : 0x40; vst->start_time = 0; @@ -186,16 +158,6 @@ static int hnm_read_packet(AVFormatContext *s, AVPacket *pkt) return ret; } -static int hnm_read_close(AVFormatContext *s) -{ - Hnm4DemuxContext *hnm = s->priv_data; - - if (hnm->vpkt.size > 0) - av_packet_unref(&hnm->vpkt); - - return 0; -} - AVInputFormat ff_hnm_demuxer = { .name = "hnm", .long_name = NULL_IF_CONFIG_SMALL("Cryo HNM v4"), @@ -203,6 +165,5 @@ AVInputFormat ff_hnm_demuxer = { .read_probe = hnm_probe, .read_header = hnm_read_header, .read_packet = hnm_read_packet, - .read_close = hnm_read_close, .flags = AVFMT_NO_BYTE_SEEK | AVFMT_NOGENSEARCH | AVFMT_NOBINSEARCH };
The hnm demuxer's context struct contained lots of fields that are write-only variables or that are not used outside of parsing the header and that can therefore be replaced by local variables of hnm_read_header(). This commit removes all of these from the context; the second type has been replaced by local variables. An AVPacket (that was initialized when reading the header and for which dead code to unreference it existed in hnm_read_close()) is among the removed things. Removing it allowed to remove hnm_read_close() altogether and also removes another instance of usage of sizeof(AVPacket). Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/hnm.c | 63 +++++++++-------------------------------------- 1 file changed, 12 insertions(+), 51 deletions(-)