[FFmpeg-devel] [PATCH] dvbsub fix transcoding
Michael Niedermayer
michaelni at gmx.at
Sun Jun 15 20:37:38 CEST 2014
On Sat, Jun 14, 2014 at 06:04:46PM +0530, Anshul Maheshwari wrote:
> On Thu, Jun 5, 2014 at 5:16 AM, Michael Niedermayer <michaelni at gmx.at>
> wrote:
>
> > On Thu, Jun 05, 2014 at 12:02:44AM +0530, Anshul wrote:
> > > On June 4, 2014 11:08:37 PM IST, Michael Niedermayer <michaelni at gmx.at>
> > wrote:
> > > >On Fri, May 23, 2014 at 03:50:05PM +0530, anshul wrote:
> > > >> On 05/22/2014 06:43 PM, Wim Vander Schelden wrote:
> > > >> >On Thu, May 22, 2014 at 3:01 PM, Wim Vander Schelden
> > > ><lists at fixnum.org>wrote:
> > > >> >
> > > >> >>These subtitles not disappearing is exactly the issue that's
> > > >introduced by
> > > >> >>sending the second (empty, "clearing") packet (by removing the nb =
> > > >2).
> > > >> you mean not sending second packet here.
> > > >> >>Note that you are now relying on the DVB subtitle timeout value,
> > > >which is
> > > >> >>only for "safety" purposes (ie, the encoder crashed), and is only
> > > >required
> > > >> >>by the specification to be followed with a 5 second accuracy (so it
> > > >is
> > > >> >>completely useless for functioning subtitles)!
> > > >> >>
> > > >> agree its for safety purpose, but as you see -0/+5 accuracy means
> > > >> that it might be possible that subtitle
> > > >> are on screen +5 second ie more then required time. so using timeout
> > > >> does not have any harm.
> > > >> though relying only on it, is harmful.
> > > >> following is copy pasted from ETSI EN 300 743 V1.3.1 (2006-11)
> > > >> page_time_out: The period, expressed in seconds, after which a page
> > > >> instance is no longer valid and consequently shall
> > > >> be erased from the screen, should it not have been redefined before
> > > >> that. The time-out period starts when the page
> > > >> instance is first displayed. The page_time_out value applies to each
> > > >> page instance until its value is redefined. The
> > > >> purpose of the time-out period is to avoid a page instance remaining
> > > >> on the screen "for ever" if the IRD happens to have
> > > >> missed the redefinition or deletion of the page instance. The
> > > >> time-out period does not need to be counted very
> > > >> accurately by the IRD: a reaction accuracy of -0/+5 s is accurate
> > > >enough.
> > > >>
> > > >> >I seem to have misformulated my ordering issue in my earlier mail:
> > > >the
> > > >> >problem occurs when subtitle X only disappears after subtitle X+1
> > > >should
> > > >> >start being displayed, as the "clear" packet for X is emitted
> > > >immediately
> > > >> >after the "show" packet for X, so before the "show" packet for X+1.
> > > >> >
> > > >> yes agree :)
> > > >>
> > > >>
> > > >> So problem is
> > > >> end_display_time is not reliable in case of dvbsub decoder,
> > > >> since end_display_time in dvbsub decoder is calculated using
> > > >page_time_out.
> > > >> which is only for safety purpose.
> > > >>
> > > >> I am trying to fix end_display_time in dvbsub decoder.
> > > >> I want to know should i cache the subtitle till i get clear_display
> > > >> region/overlap region
> > > >> or should i try to overwrite end_display_time when i have recieved
> > > >> some new region.
> > > >>
> > > >> just now i have attached patch which stop second loop only when
> > > >> stream was decoded by
> > > >> dvbsubtile.
> > > >>
> > > >> -Anshul
> > > >
> > > >> ffmpeg.c | 4 +++-
> > > >> libavcodec/dvbsub.c | 2 +-
> > > >> 2 files changed, 4 insertions(+), 2 deletions(-)
> > > >> 5d9736343922cc0aad0fe3124943e5cf7406b2c6
> > > >0001-dvbsub-fix-transcoding.patch
> > > >> From 946dc41a84a6b17d64445acb2424e18774e7f1ac Mon Sep 17 00:00:00
> > > >2001
> > > >> From: Anshul Maheshwari <er.anshul.maheshwari at gmail.com>
> > > >> Date: Fri, 23 May 2014 15:48:12 +0530
> > > >> Subject: [PATCH] dvbsub fix transcoding
> > > >>
> > > >> Fix Ticket #2024
> > > >> ---
> > > >> ffmpeg.c | 4 +++-
> > > >> libavcodec/dvbsub.c | 2 +-
> > > >> 2 files changed, 4 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/ffmpeg.c b/ffmpeg.c
> > > >> index 5299f0e..d9539ee 100644
> > > >> --- a/ffmpeg.c
> > > >> +++ b/ffmpeg.c
> > > >> @@ -770,6 +770,7 @@ static void do_subtitle_out(AVFormatContext *s,
> > > >> int subtitle_out_max_size = 1024 * 1024;
> > > >> int subtitle_out_size, nb, i;
> > > >> AVCodecContext *enc;
> > > >> + AVCodecContext *dec;
> > > >> AVPacket pkt;
> > > >> int64_t pts;
> > > >>
> > > >> @@ -781,6 +782,7 @@ static void do_subtitle_out(AVFormatContext *s,
> > > >> }
> > > >>
> > > >> enc = ost->st->codec;
> > > >> + dec = ist->st->codec;
> > > >>
> > > >> if (!subtitle_out) {
> > > >> subtitle_out = av_malloc(subtitle_out_max_size);
> > > >> @@ -789,7 +791,7 @@ static void do_subtitle_out(AVFormatContext *s,
> > > >> /* Note: DVB subtitle need one packet to draw them and one other
> > > >> packet to clear them */
> > > >> /* XXX: signal it in the codec context ? */
> > > >> - if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
> > > >> + if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE && dec->codec_id
> > > >!= AV_CODEC_ID_DVB_SUBTITLE)
> > > >
> > > >
> > > >> nb = 2;
> > > >
> > > >this case (independant of this patch) needs some kind of que
> > > >(either inside the encoder or elsewhere) to ensure the clear packet
> > > >is correctly ordered in relation to the next "add" packet
> > > >there was a patch about this but i think no new version was posted
> > > >after reviews
> > > >
> > > >
> > >
> > > Actually there was some disscussion going on for encode_subtitle2 api, I
> > was waiting for that since here we need to give 2 packets or frame of
> > subtitle out. One for start time another for end time.
> > >
> > > >> else
> > > >> nb = 1;
> > > >> diff --git a/libavcodec/dvbsub.c b/libavcodec/dvbsub.c
> > > >> index f6b46e6..f4b4efa 100644
> > > >> --- a/libavcodec/dvbsub.c
> > > >> +++ b/libavcodec/dvbsub.c
> > > >> @@ -268,7 +268,7 @@ static int
> > > >encode_dvb_subtitles(DVBSubtitleContext *s,
> > > >> bytestream_put_be16(&q, page_id);
> > > >> pseg_len = q;
> > > >> q += 2; /* segment length */
> > > >> - *q++ = 30; /* page_timeout (seconds) */
> > > >> + *q++ = ceil(h->end_display_time /1000); /* page_timeout
> > > >(seconds) */
> > > >
> > > >this would need a check for the value fitting in a byte (0..255)
> > > >
> > >
> > > I have not seen more then 30 here,but I haven't seen all videos yet. :)
> > > So if values more then 255 comes should I put 255 there.
> > >
> > > >also this only sort of fixes/hacks around the dvb->dvb case, there
> > > >still wont be correctly set durations for dvb->something else
> > > >I think, unless iam missing something
> > > >
> > > If you are talking about whole patch
> > > This fix if end time is not reliable, thats the case with dvb decoder.
> > > Since nb=2 is working on basis of end time only.
> > > So it works for dvb-> dvb and reliable_end_time-> dvb
> > >
> > >
> > > Some thing I would like to ask that, should I work on the inputs for
> > second patch or first one.
> >
> > I think the durations need to be optionally calculated
> > this patch here still wont give correct dvb->dvb as is because the
> > end times currently arent correctm they are just the timeout values
> >
> > [...]
> > --
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Concerning the gods, I have no means of knowing whether they exist or not
> > or of what sort they may be, because of the obscurity of the subject, and
> > the brevity of human life -- Protagoras
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
>
>
> I have attached patch as per disscussion
applied parts of it, review of remainder below:
> diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
> index e6e746d..c213524 100644
> --- a/libavcodec/dvbsubdec.c
> +++ b/libavcodec/dvbsubdec.c
> @@ -234,6 +234,9 @@ typedef struct DVBSubContext {
>
> int version;
> int time_out;
> + int compute_edt; /**< if 1 end display time calculated using pts
> + if 0 (Default) calculated using pts */
"using pts" in both cases is wrong
> + int64_t prev_start;
> DVBSubRegion *region_list;
> DVBSubCLUT *clut_list;
> DVBSubObject *object_list;
> @@ -771,7 +774,8 @@ static void save_subtitle_set(AVCodecContext *avctx, AVSubtitle *sub)
> int i;
> int offset_x=0, offset_y=0;
>
> - sub->end_display_time = ctx->time_out * 1000;
> + if(ctx->compute_edt == 0)
> + sub->end_display_time = ctx->time_out * 1000;
>
> if (display_def) {
> offset_x = display_def->x;
> @@ -786,6 +790,8 @@ static void save_subtitle_set(AVCodecContext *avctx, AVSubtitle *sub)
> }
>
> if (sub->num_rects > 0) {
> + if(ctx->compute_edt == 1 && ctx->prev_start)
>
prev_start == 0 is the wrong thing to check for here, 0 shouldnt be
special, it could be a valid timestamp
> + sub->end_display_time = av_rescale_q((sub->pts - ctx->prev_start )/90, AV_TIME_BASE_Q, avctx->time_base) - 1;
the /90 looks wrong as well
rescaling happen between 2 timebases, there should not be a 3rd factor
dividing the input before av_rescale_q()
> sub->rects = av_mallocz_array(sizeof(*sub->rects), sub->num_rects);
> for(i=0; i<sub->num_rects; i++)
> sub->rects[i] = av_mallocz(sizeof(*sub->rects[i]));
> @@ -837,6 +843,8 @@ static void save_subtitle_set(AVCodecContext *avctx, AVSubtitle *sub)
> i++;
> }
> }
> + if(ctx->compute_edt == 1)
> + FFSWAP(int64_t,ctx->prev_start,sub->pts);
> }
>
IIRC there can be "things" like for example differnent languages and
for example the begin of the english subtitle doesnt signify the end
of the previous german one
also after this patch the "int *data_size," from the dvbsub_decode()
function will be wrongly set
[...]
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -1999,7 +1999,15 @@ fail:
static int transcode_subtitles(InputStream *ist, AVPacket *pkt, int *got_output)
{
AVSubtitle subtitle;
- int i, ret = avcodec_decode_subtitle2(ist->dec_ctx,
+ AVDictionary *options = NULL;
+ int i, ret = 0;
+
+ if(ist->dec_ctx->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
+ {
+ av_dict_set(&options, "compute_edt", "1", 0);
+ av_opt_set_dict2(ist->dec_ctx->priv_data, &options, AV_OPT_SEARCH_CHILDREN);
+ }
+ ret = avcodec_decode_subtitle2(ist->dec_ctx,
&subtitle, got_output, pkt);
this option should be set when opening the codec not when decoding
repeatly for each packet
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140615/9292228b/attachment.asc>
More information about the ffmpeg-devel
mailing list