| Message ID | 20160927180305.6132-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 9/27/2016 3:03 PM, James Almer wrote: > It's listed as supported in both https://www.webmproject.org/docs/container/ > and https://matroska.org/technical/specs/index.html > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/matroskaenc.c | 41 +++++++++++++++++++++-------------------- > 1 file changed, 21 insertions(+), 20 deletions(-) Ping.
Hi, > On Oct 2, 2016, at 7:14 PM, James Almer <jamrial@gmail.com> wrote: > > On 9/27/2016 3:03 PM, James Almer wrote: >> It's listed as supported in both https://www.webmproject.org/docs/container/ >> and https://matroska.org/technical/specs/index.html >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/matroskaenc.c | 41 +++++++++++++++++++++-------------------- >> 1 file changed, 21 insertions(+), 20 deletions(-) > > Ping. Untested but from a read through, the patch looks good. FlagInterlaced is supported in both Matroska and webM, whereas FieldOrder (a new field from the CELLAR work on Matroska) is only supported in Matroska. IMHO though I think that FlagInterlaced without information on FieldOrder is not so useful and that the webM project should consider adopting FieldOrder as well. I'd prefer to see a patch to webM to consider adding FieldOrder to the format rather than see FieldOrder removed from the webM muxer in FFmpeg. Dave Rice
On 10/3/2016 12:27 PM, Dave Rice wrote: > Hi, > >> On Oct 2, 2016, at 7:14 PM, James Almer <jamrial@gmail.com> wrote: >> >> On 9/27/2016 3:03 PM, James Almer wrote: >>> It's listed as supported in both https://www.webmproject.org/docs/container/ >>> and https://matroska.org/technical/specs/index.html >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavformat/matroskaenc.c | 41 +++++++++++++++++++++-------------------- >>> 1 file changed, 21 insertions(+), 20 deletions(-) >> >> Ping. > > Untested but from a read through, the patch looks good. FlagInterlaced is supported in both Matroska and webM, whereas FieldOrder (a new field from the CELLAR work on Matroska) is only supported in Matroska. IMHO though I think that FlagInterlaced without information on FieldOrder is not so useful and that the webM project should consider adopting FieldOrder as well. I'd prefer to see a patch to webM to consider adding FieldOrder to the format rather than see FieldOrder removed from the webM muxer in FFmpeg. FieldOrder is not being removed from the WebM muxer with this patch. It, alongside FlagInterlaced, was never written by it. Only by the Matroska muxer. This patch follows the current spec and adds FlagInterlaced to WebM by moving the mode check to only filter out FieldOrder instead of the two elements when targeting WebM. If Google ever changes the spec to also support FieldOrder then adding it will be as simple as removing the mode check. > > Dave Rice > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 10/3/2016 1:21 PM, James Almer wrote: > On 10/3/2016 12:27 PM, Dave Rice wrote: >> Hi, >> >>> On Oct 2, 2016, at 7:14 PM, James Almer <jamrial@gmail.com> wrote: >>> >>> On 9/27/2016 3:03 PM, James Almer wrote: >>>> It's listed as supported in both https://www.webmproject.org/docs/container/ >>>> and https://matroska.org/technical/specs/index.html >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavformat/matroskaenc.c | 41 +++++++++++++++++++++-------------------- >>>> 1 file changed, 21 insertions(+), 20 deletions(-) >>> >>> Ping. >> >> Untested but from a read through, the patch looks good. FlagInterlaced is supported in both Matroska and webM, whereas FieldOrder (a new field from the CELLAR work on Matroska) is only supported in Matroska. IMHO though I think that FlagInterlaced without information on FieldOrder is not so useful and that the webM project should consider adopting FieldOrder as well. I'd prefer to see a patch to webM to consider adding FieldOrder to the format rather than see FieldOrder removed from the webM muxer in FFmpeg. > > FieldOrder is not being removed from the WebM muxer with this patch. It, > alongside FlagInterlaced, was never written by it. Only by the Matroska > muxer. > This patch follows the current spec and adds FlagInterlaced to WebM by > moving the mode check to only filter out FieldOrder instead of the two > elements when targeting WebM. > > If Google ever changes the spec to also support FieldOrder then adding > it will be as simple as removing the mode check. > Pushed, thanks.
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 3eeb09b..6cb0376 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -791,7 +791,7 @@ static int mkv_write_video_color(AVIOContext *pb, AVCodecParameters *par, AVStre return 0; } -static void mkv_write_field_order(AVIOContext *pb, +static void mkv_write_field_order(AVIOContext *pb, int mode, enum AVFieldOrder field_order) { switch (field_order) { @@ -809,23 +809,25 @@ static void mkv_write_field_order(AVIOContext *pb, case AV_FIELD_BT: put_ebml_uint(pb, MATROSKA_ID_VIDEOFLAGINTERLACED, MATROSKA_VIDEO_INTERLACE_FLAG_INTERLACED); - switch (field_order) { - case AV_FIELD_TT: - put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, - MATROSKA_VIDEO_FIELDORDER_TT); - break; - case AV_FIELD_BB: - put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, - MATROSKA_VIDEO_FIELDORDER_BB); - break; - case AV_FIELD_TB: - put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, - MATROSKA_VIDEO_FIELDORDER_TB); - break; - case AV_FIELD_BT: - put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, - MATROSKA_VIDEO_FIELDORDER_BT); - break; + if (mode != MODE_WEBM) { + switch (field_order) { + case AV_FIELD_TT: + put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, + MATROSKA_VIDEO_FIELDORDER_TT); + break; + case AV_FIELD_BB: + put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, + MATROSKA_VIDEO_FIELDORDER_BB); + break; + case AV_FIELD_TB: + put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, + MATROSKA_VIDEO_FIELDORDER_TB); + break; + case AV_FIELD_BT: + put_ebml_uint(pb, MATROSKA_ID_VIDEOFIELDORDER, + MATROSKA_VIDEO_FIELDORDER_BT); + break; + } } } } @@ -1088,8 +1090,7 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, put_ebml_uint (pb, MATROSKA_ID_VIDEOPIXELWIDTH , par->width); put_ebml_uint (pb, MATROSKA_ID_VIDEOPIXELHEIGHT, par->height); - if (mkv->mode != MODE_WEBM) - mkv_write_field_order(pb, par->field_order); + mkv_write_field_order(pb, mkv->mode, par->field_order); // check both side data and metadata for stereo information, // write the result to the bitstream if any is found
It's listed as supported in both https://www.webmproject.org/docs/container/ and https://matroska.org/technical/specs/index.html Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/matroskaenc.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)