| Message ID | 20170308194649.5728-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Mar 8, 2017 at 2:46 PM, James Almer <jamrial@gmail.com> wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/matroskaenc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index 1605f0cafe..0ee927d63e 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -918,6 +918,72 @@ static int mkv_write_video_color(AVIOContext *pb, AVCodecParameters *par, AVStre > return 0; > } > > +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st) { > + int side_data_size = 0; > + const AVSphericalMapping *spherical = > + (const AVSphericalMapping*) av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, > + &side_data_size); > + > + if (side_data_size == sizeof(AVSphericalMapping)) { I don't think you have to check for this (and checking sizeof() of this struct outside lavu breaks ABI), it's enough to check that size != 0 > + AVIOContext *dyn_cp; > + uint8_t *projection_ptr; > + int ret, projection_size; > + > + ret = avio_open_dyn_buf(&dyn_cp); > + if (ret < 0) > + return ret; > + > + switch (spherical->projection) { > + case AV_SPHERICAL_EQUIRECTANGULAR_TILE: > + { > + uint8_t private[20]; > + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, > + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); > + AV_WB32(private + 0, 0); // version + flags > + AV_WB32(private + 4, spherical->bound_top); > + AV_WB32(private + 8, spherical->bound_bottom); > + AV_WB32(private + 12, spherical->bound_left); > + AV_WB32(private + 16, spherical->bound_right); I'd feel better if you could use bytestream functions > + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); > + break; > + } > + case AV_SPHERICAL_EQUIRECTANGULAR: > + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, > + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); > + break; > + case AV_SPHERICAL_CUBEMAP: > + { > + uint8_t private[12]; > + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, > + MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP); > + AV_WB32(private + 0, 0); // version + flags > + AV_WB32(private + 4, 0); // layout > + AV_WB32(private + 8, spherical->padding); > + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); > + break; > + } > + default: > + // TODO: Mesh projection once implemented in AVSphericalMapping a little av_log message to warn about this? > + goto end; > + } > + > + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (double)spherical->yaw / (1 << 16)); > + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (double)spherical->pitch / (1 << 16)); > + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (double)spherical->roll / (1 << 16)); does the matroska spec require plain integers? spherical->{yaw,pitch,roll} are in 16.16 so there should be no need of extra conversions > + > +end: > + projection_size = avio_close_dyn_buf(dyn_cp, &projection_ptr); > + if (projection_size) { > + ebml_master projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, projection_size); > + avio_write(pb, projection_ptr, projection_size); > + end_ebml_master(pb, projection); > + } > + av_freep(&projection_ptr); > + } > + > + return 0; > +} > + > static void mkv_write_field_order(AVIOContext *pb, int mode, > enum AVFieldOrder field_order) > { > @@ -1268,6 +1334,9 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, > ret = mkv_write_video_color(pb, par, st); > if (ret < 0) > return ret; > + ret = mkv_write_video_projection(pb, st); > + if (ret < 0) > + return ret; > end_ebml_master(pb, subinfo); > break; > > -- > 2.12.0 >
On 3/8/2017 5:08 PM, Vittorio Giovara wrote: > On Wed, Mar 8, 2017 at 2:46 PM, James Almer <jamrial@gmail.com> wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/matroskaenc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 69 insertions(+) >> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >> index 1605f0cafe..0ee927d63e 100644 >> --- a/libavformat/matroskaenc.c >> +++ b/libavformat/matroskaenc.c >> @@ -918,6 +918,72 @@ static int mkv_write_video_color(AVIOContext *pb, AVCodecParameters *par, AVStre >> return 0; >> } >> >> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st) { >> + int side_data_size = 0; >> + const AVSphericalMapping *spherical = >> + (const AVSphericalMapping*) av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, >> + &side_data_size); >> + >> + if (side_data_size == sizeof(AVSphericalMapping)) { > > I don't think you have to check for this (and checking sizeof() of > this struct outside lavu breaks ABI), it's enough to check that size > != 0 Ok. You'll probably also have to do the same on libavformat/dump.c, for that matter. > >> + AVIOContext *dyn_cp; >> + uint8_t *projection_ptr; >> + int ret, projection_size; >> + >> + ret = avio_open_dyn_buf(&dyn_cp); >> + if (ret < 0) >> + return ret; >> + >> + switch (spherical->projection) { >> + case AV_SPHERICAL_EQUIRECTANGULAR_TILE: >> + { >> + uint8_t private[20]; >> + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, >> + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); >> + AV_WB32(private + 0, 0); // version + flags >> + AV_WB32(private + 4, spherical->bound_top); >> + AV_WB32(private + 8, spherical->bound_bottom); >> + AV_WB32(private + 12, spherical->bound_left); >> + AV_WB32(private + 16, spherical->bound_right); > > I'd feel better if you could use bytestream functions Seems like pointless complexity to me, but sure. > >> + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); >> + break; >> + } >> + case AV_SPHERICAL_EQUIRECTANGULAR: >> + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, >> + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); >> + break; >> + case AV_SPHERICAL_CUBEMAP: >> + { >> + uint8_t private[12]; >> + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, >> + MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP); >> + AV_WB32(private + 0, 0); // version + flags >> + AV_WB32(private + 4, 0); // layout >> + AV_WB32(private + 8, spherical->padding); >> + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); >> + break; >> + } >> + default: >> + // TODO: Mesh projection once implemented in AVSphericalMapping > > a little av_log message to warn about this? This is a muxer, spherical->projection will be one of the supported enums. This default case is just to prevent potential compiler warnings and can be removed. What needs a log message is the demuxer. > >> + goto end; >> + } >> + >> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (double)spherical->yaw / (1 << 16)); >> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (double)spherical->pitch / (1 << 16)); >> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (double)spherical->roll / (1 << 16)); > > does the matroska spec require plain integers? > spherical->{yaw,pitch,roll} are in 16.16 so there should be no need of > extra conversions Matroska stores these three as double precision fp values. > >> + >> +end: >> + projection_size = avio_close_dyn_buf(dyn_cp, &projection_ptr); >> + if (projection_size) { >> + ebml_master projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, projection_size); >> + avio_write(pb, projection_ptr, projection_size); >> + end_ebml_master(pb, projection); >> + } >> + av_freep(&projection_ptr); >> + } >> + >> + return 0; >> +} >> + >> static void mkv_write_field_order(AVIOContext *pb, int mode, >> enum AVFieldOrder field_order) >> { >> @@ -1268,6 +1334,9 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, >> ret = mkv_write_video_color(pb, par, st); >> if (ret < 0) >> return ret; >> + ret = mkv_write_video_projection(pb, st); >> + if (ret < 0) >> + return ret; >> end_ebml_master(pb, subinfo); >> break; >> >> -- >> 2.12.0 >> > > >
On Wed, Mar 8, 2017 at 4:05 PM, James Almer <jamrial@gmail.com> wrote: > On 3/8/2017 5:08 PM, Vittorio Giovara wrote: >> On Wed, Mar 8, 2017 at 2:46 PM, James Almer <jamrial@gmail.com> wrote: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavformat/matroskaenc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 69 insertions(+) >>> >>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >>> index 1605f0cafe..0ee927d63e 100644 >>> --- a/libavformat/matroskaenc.c >>> +++ b/libavformat/matroskaenc.c >>> @@ -918,6 +918,72 @@ static int mkv_write_video_color(AVIOContext *pb, AVCodecParameters *par, AVStre >>> return 0; >>> } >>> >>> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st) { >>> + int side_data_size = 0; >>> + const AVSphericalMapping *spherical = >>> + (const AVSphericalMapping*) av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, >>> + &side_data_size); >>> + >>> + if (side_data_size == sizeof(AVSphericalMapping)) { >> >> I don't think you have to check for this (and checking sizeof() of >> this struct outside lavu breaks ABI), it's enough to check that size >> != 0 > > Ok. > > You'll probably also have to do the same on libavformat/dump.c, for > that matter. This is valid for most side data there, somebody should do a thorough cleanup >>> + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); >>> + break; >>> + } >>> + case AV_SPHERICAL_EQUIRECTANGULAR: >>> + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, >>> + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); >>> + break; >>> + case AV_SPHERICAL_CUBEMAP: >>> + { >>> + uint8_t private[12]; >>> + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, >>> + MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP); >>> + AV_WB32(private + 0, 0); // version + flags >>> + AV_WB32(private + 4, 0); // layout >>> + AV_WB32(private + 8, spherical->padding); >>> + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); >>> + break; >>> + } >>> + default: >>> + // TODO: Mesh projection once implemented in AVSphericalMapping >> >> a little av_log message to warn about this? > > This is a muxer, spherical->projection will be one of the supported enums. > This default case is just to prevent potential compiler warnings and can > be removed. > > What needs a log message is the demuxer. This is a muxer, that can be created by an API user that does not know how to read documentation and who initialises spherical->projection wrong. I didn't say to error out, but I think that adding just a warning is easier to deal with than having to debug it manually. >> >>> + goto end; >>> + } >>> + >>> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (double)spherical->yaw / (1 << 16)); >>> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (double)spherical->pitch / (1 << 16)); >>> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (double)spherical->roll / (1 << 16)); >> >> does the matroska spec require plain integers? >> spherical->{yaw,pitch,roll} are in 16.16 so there should be no need of >> extra conversions > > Matroska stores these three as double precision fp values. nice
On 3/8/2017 7:09 PM, Vittorio Giovara wrote: > On Wed, Mar 8, 2017 at 4:05 PM, James Almer <jamrial@gmail.com> wrote: >> On 3/8/2017 5:08 PM, Vittorio Giovara wrote: >>> On Wed, Mar 8, 2017 at 2:46 PM, James Almer <jamrial@gmail.com> wrote: >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavformat/matroskaenc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 69 insertions(+) >>>> >>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >>>> index 1605f0cafe..0ee927d63e 100644 >>>> --- a/libavformat/matroskaenc.c >>>> +++ b/libavformat/matroskaenc.c >>>> @@ -918,6 +918,72 @@ static int mkv_write_video_color(AVIOContext *pb, AVCodecParameters *par, AVStre >>>> return 0; >>>> } >>>> >>>> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st) { >>>> + int side_data_size = 0; >>>> + const AVSphericalMapping *spherical = >>>> + (const AVSphericalMapping*) av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, >>>> + &side_data_size); >>>> + >>>> + if (side_data_size == sizeof(AVSphericalMapping)) { >>> >>> I don't think you have to check for this (and checking sizeof() of >>> this struct outside lavu breaks ABI), it's enough to check that size >>> != 0 >> >> Ok. >> >> You'll probably also have to do the same on libavformat/dump.c, for >> that matter. > > This is valid for most side data there, somebody should do a thorough cleanup > >>>> + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); >>>> + break; >>>> + } >>>> + case AV_SPHERICAL_EQUIRECTANGULAR: >>>> + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, >>>> + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); >>>> + break; >>>> + case AV_SPHERICAL_CUBEMAP: >>>> + { >>>> + uint8_t private[12]; >>>> + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, >>>> + MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP); >>>> + AV_WB32(private + 0, 0); // version + flags >>>> + AV_WB32(private + 4, 0); // layout >>>> + AV_WB32(private + 8, spherical->padding); >>>> + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); >>>> + break; >>>> + } >>>> + default: >>>> + // TODO: Mesh projection once implemented in AVSphericalMapping >>> >>> a little av_log message to warn about this? >> >> This is a muxer, spherical->projection will be one of the supported enums. >> This default case is just to prevent potential compiler warnings and can >> be removed. >> >> What needs a log message is the demuxer. > > This is a muxer, that can be created by an API user that does not know > how to read documentation and who initialises spherical->projection > wrong. I didn't say to error out, but I think that adding just a > warning is easier to deal with than having to debug it manually. Alright, will add one before pushing. > >>> >>>> + goto end; >>>> + } >>>> + >>>> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (double)spherical->yaw / (1 << 16)); >>>> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (double)spherical->pitch / (1 << 16)); >>>> + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (double)spherical->roll / (1 << 16)); >>> >>> does the matroska spec require plain integers? >>> spherical->{yaw,pitch,roll} are in 16.16 so there should be no need of >>> extra conversions >> >> Matroska stores these three as double precision fp values. > > nice >
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 1605f0cafe..0ee927d63e 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -918,6 +918,72 @@ static int mkv_write_video_color(AVIOContext *pb, AVCodecParameters *par, AVStre return 0; } +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st) { + int side_data_size = 0; + const AVSphericalMapping *spherical = + (const AVSphericalMapping*) av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, + &side_data_size); + + if (side_data_size == sizeof(AVSphericalMapping)) { + AVIOContext *dyn_cp; + uint8_t *projection_ptr; + int ret, projection_size; + + ret = avio_open_dyn_buf(&dyn_cp); + if (ret < 0) + return ret; + + switch (spherical->projection) { + case AV_SPHERICAL_EQUIRECTANGULAR_TILE: + { + uint8_t private[20]; + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); + AV_WB32(private + 0, 0); // version + flags + AV_WB32(private + 4, spherical->bound_top); + AV_WB32(private + 8, spherical->bound_bottom); + AV_WB32(private + 12, spherical->bound_left); + AV_WB32(private + 16, spherical->bound_right); + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); + break; + } + case AV_SPHERICAL_EQUIRECTANGULAR: + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, + MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); + break; + case AV_SPHERICAL_CUBEMAP: + { + uint8_t private[12]; + put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, + MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP); + AV_WB32(private + 0, 0); // version + flags + AV_WB32(private + 4, 0); // layout + AV_WB32(private + 8, spherical->padding); + put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private)); + break; + } + default: + // TODO: Mesh projection once implemented in AVSphericalMapping + goto end; + } + + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (double)spherical->yaw / (1 << 16)); + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (double)spherical->pitch / (1 << 16)); + put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (double)spherical->roll / (1 << 16)); + +end: + projection_size = avio_close_dyn_buf(dyn_cp, &projection_ptr); + if (projection_size) { + ebml_master projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, projection_size); + avio_write(pb, projection_ptr, projection_size); + end_ebml_master(pb, projection); + } + av_freep(&projection_ptr); + } + + return 0; +} + static void mkv_write_field_order(AVIOContext *pb, int mode, enum AVFieldOrder field_order) { @@ -1268,6 +1334,9 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, ret = mkv_write_video_color(pb, par, st); if (ret < 0) return ret; + ret = mkv_write_video_projection(pb, st); + if (ret < 0) + return ret; end_ebml_master(pb, subinfo); break;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/matroskaenc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)