| Message ID | 20200522012431.29918-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 68dd1e6a57c8645656278b70f870a85e3789ec27 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/matroskaenc: Don't use stream side-data size | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 5/21/2020 10:24 PM, Andreas Rheinhardt wrote: > av_stream_get_side_data() tells the caller whether a stream has side > data of a specific type; if present it can also tell the caller the size > of the side data via an optional argument. The Matroska muxer always > used this optional argument, although it doesn't really need the size, > as the relevant side-data are not buffers, but structures. So change > this. > > Furthermore, relying on the size also made the code susceptible to > a quirk of av_stream_get_side_data(): It only sets the size argument if > it found side data of the desired type. Sounds like something that should be fixed instead. av_packet_get_side_data() sets the size argument to 0 if it doesn't find the requested side data type. This function should do the same. > mkv_write_video_color() checks > for side-data twice with the same variable for the size without resetting > the size in between; if the second type of side-data isn't present, the > size will still be what it was after the first call. This was not > dangerous in practice, as the check for the existence of the second > side-data compared the size with the expected size, so it would only be > problematic if lots of elements were to be added to AVContentLightMetadata. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/matroskaenc.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index fccfee539a..f5968c17b4 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -835,7 +835,6 @@ static void mkv_write_video_color(AVIOContext *pb, const AVStream *st, > * plus another byte to stay clear of the end. */ > uint8_t colour[(2 + 1 + 8) * 18 + (2 + 1) + 1]; > AVIOContext buf, *dyn_cp = &buf; > - int side_data_size = 0; > int colorinfo_size; > const uint8_t *side_data; > > @@ -868,8 +867,8 @@ static void mkv_write_video_color(AVIOContext *pb, const AVStream *st, > } > > side_data = av_stream_get_side_data(st, AV_PKT_DATA_CONTENT_LIGHT_LEVEL, > - &side_data_size); > - if (side_data_size) { > + NULL); > + if (side_data) { > const AVContentLightMetadata *metadata = > (const AVContentLightMetadata*)side_data; > put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOCOLORMAXCLL, metadata->MaxCLL); > @@ -877,8 +876,8 @@ static void mkv_write_video_color(AVIOContext *pb, const AVStream *st, > } > > side_data = av_stream_get_side_data(st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA, > - &side_data_size); > - if (side_data_size == sizeof(AVMasteringDisplayMetadata)) { > + NULL); > + if (side_data) { > ebml_master meta_element = start_ebml_master( > dyn_cp, MATROSKA_ID_VIDEOCOLORMASTERINGMETA, 10 * (2 + 1 + 8)); > const AVMasteringDisplayMetadata *metadata = > @@ -919,14 +918,13 @@ static void mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb, > const AVStream *st) > { > ebml_master projection; > - int side_data_size = 0; > uint8_t private[20]; > > const AVSphericalMapping *spherical = > (const AVSphericalMapping *)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, > - &side_data_size); > + NULL); > > - if (!side_data_size) > + if (!spherical) > return; > > if (spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR && > @@ -1028,7 +1026,6 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb, > const AVDictionaryEntry *tag; > MatroskaVideoStereoModeType format = MATROSKA_VIDEO_STEREOMODE_TYPE_NB; > const AVStereo3D *stereo; > - int side_data_size = 0; > > *h_width = 1; > *h_height = 1; > @@ -1052,8 +1049,8 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb, > } > > stereo = (const AVStereo3D*)av_stream_get_side_data(st, AV_PKT_DATA_STEREO3D, > - &side_data_size); > - if (side_data_size >= sizeof(AVStereo3D)) { > + NULL); > + if (stereo) { > switch (stereo->type) { > case AV_STEREO3D_2D: > format = MATROSKA_VIDEO_STEREOMODE_TYPE_MONO; >
On 5/22/2020 12:31 AM, James Almer wrote: > On 5/21/2020 10:24 PM, Andreas Rheinhardt wrote: >> av_stream_get_side_data() tells the caller whether a stream has side >> data of a specific type; if present it can also tell the caller the size >> of the side data via an optional argument. The Matroska muxer always >> used this optional argument, although it doesn't really need the size, >> as the relevant side-data are not buffers, but structures. So change >> this. >> >> Furthermore, relying on the size also made the code susceptible to >> a quirk of av_stream_get_side_data(): It only sets the size argument if >> it found side data of the desired type. > > Sounds like something that should be fixed instead. > av_packet_get_side_data() sets the size argument to 0 if it doesn't find > the requested side data type. This function should do the same. Right, you did as much in patch 3/3. This patch is worth applying either way seeing it removes things like the API violating usage of sizeof(AVMasteringDisplayMetadata), so LGTM.
James Almer: > On 5/21/2020 10:24 PM, Andreas Rheinhardt wrote: >> av_stream_get_side_data() tells the caller whether a stream has side >> data of a specific type; if present it can also tell the caller the size >> of the side data via an optional argument. The Matroska muxer always >> used this optional argument, although it doesn't really need the size, >> as the relevant side-data are not buffers, but structures. So change >> this. >> >> Furthermore, relying on the size also made the code susceptible to >> a quirk of av_stream_get_side_data(): It only sets the size argument if >> it found side data of the desired type. > > Sounds like something that should be fixed instead. > av_packet_get_side_data() sets the size argument to 0 if it doesn't find > the requested side data type. This function should do the same. > The third patch [1] in this patchset does exactly this. - Andreas [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-May/263088.html
James Almer: > On 5/22/2020 12:31 AM, James Almer wrote: >> On 5/21/2020 10:24 PM, Andreas Rheinhardt wrote: >>> av_stream_get_side_data() tells the caller whether a stream has side >>> data of a specific type; if present it can also tell the caller the size >>> of the side data via an optional argument. The Matroska muxer always >>> used this optional argument, although it doesn't really need the size, >>> as the relevant side-data are not buffers, but structures. So change >>> this. >>> >>> Furthermore, relying on the size also made the code susceptible to >>> a quirk of av_stream_get_side_data(): It only sets the size argument if >>> it found side data of the desired type. >> >> Sounds like something that should be fixed instead. >> av_packet_get_side_data() sets the size argument to 0 if it doesn't find >> the requested side data type. This function should do the same. > > Right, you did as much in patch 3/3. > > This patch is worth applying either way seeing it removes things like > the API violating usage of sizeof(AVMasteringDisplayMetadata), so LGTM. Ok, applied the set. Thanks. - Andreas
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index fccfee539a..f5968c17b4 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -835,7 +835,6 @@ static void mkv_write_video_color(AVIOContext *pb, const AVStream *st, * plus another byte to stay clear of the end. */ uint8_t colour[(2 + 1 + 8) * 18 + (2 + 1) + 1]; AVIOContext buf, *dyn_cp = &buf; - int side_data_size = 0; int colorinfo_size; const uint8_t *side_data; @@ -868,8 +867,8 @@ static void mkv_write_video_color(AVIOContext *pb, const AVStream *st, } side_data = av_stream_get_side_data(st, AV_PKT_DATA_CONTENT_LIGHT_LEVEL, - &side_data_size); - if (side_data_size) { + NULL); + if (side_data) { const AVContentLightMetadata *metadata = (const AVContentLightMetadata*)side_data; put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOCOLORMAXCLL, metadata->MaxCLL); @@ -877,8 +876,8 @@ static void mkv_write_video_color(AVIOContext *pb, const AVStream *st, } side_data = av_stream_get_side_data(st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA, - &side_data_size); - if (side_data_size == sizeof(AVMasteringDisplayMetadata)) { + NULL); + if (side_data) { ebml_master meta_element = start_ebml_master( dyn_cp, MATROSKA_ID_VIDEOCOLORMASTERINGMETA, 10 * (2 + 1 + 8)); const AVMasteringDisplayMetadata *metadata = @@ -919,14 +918,13 @@ static void mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb, const AVStream *st) { ebml_master projection; - int side_data_size = 0; uint8_t private[20]; const AVSphericalMapping *spherical = (const AVSphericalMapping *)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, - &side_data_size); + NULL); - if (!side_data_size) + if (!spherical) return; if (spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR && @@ -1028,7 +1026,6 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb, const AVDictionaryEntry *tag; MatroskaVideoStereoModeType format = MATROSKA_VIDEO_STEREOMODE_TYPE_NB; const AVStereo3D *stereo; - int side_data_size = 0; *h_width = 1; *h_height = 1; @@ -1052,8 +1049,8 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb, } stereo = (const AVStereo3D*)av_stream_get_side_data(st, AV_PKT_DATA_STEREO3D, - &side_data_size); - if (side_data_size >= sizeof(AVStereo3D)) { + NULL); + if (stereo) { switch (stereo->type) { case AV_STEREO3D_2D: format = MATROSKA_VIDEO_STEREOMODE_TYPE_MONO;
av_stream_get_side_data() tells the caller whether a stream has side data of a specific type; if present it can also tell the caller the size of the side data via an optional argument. The Matroska muxer always used this optional argument, although it doesn't really need the size, as the relevant side-data are not buffers, but structures. So change this. Furthermore, relying on the size also made the code susceptible to a quirk of av_stream_get_side_data(): It only sets the size argument if it found side data of the desired type. mkv_write_video_color() checks for side-data twice with the same variable for the size without resetting the size in between; if the second type of side-data isn't present, the size will still be what it was after the first call. This was not dangerous in practice, as the check for the existence of the second side-data compared the size with the expected size, so it would only be problematic if lots of elements were to be added to AVContentLightMetadata. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/matroskaenc.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)