diff mbox series

[FFmpeg-devel,3/3,v2] avformat/dashenc: always attempt to enable prft on ldash mode

Message ID 20200220162601.943-1-jamrial@gmail.com
State New
Headers show
Series Untitled series #380
Related show

Commit Message

James Almer Feb. 20, 2020, 4:26 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
Now it can be overriden if you explicitly set write_prft to 0.

 libavformat/dashenc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Anton Khirnov Feb. 24, 2020, 9:54 a.m. UTC | #1
Quoting James Almer (2020-02-20 17:26:00)
> Signed-off-by: James Almer <jamrial@gmail.com>

Commit message is now misleading since it will only enable prft if it's
not disabled.
> ---
> Now it can be overriden if you explicitly set write_prft to 0.
> 
>  libavformat/dashenc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index a52cbc9113..7032adc84d 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -1394,6 +1394,12 @@ static int dash_init(AVFormatContext *s)
>          c->frag_type = FRAG_TYPE_EVERY_FRAME;
>      }
>  
> +    if (c->write_prft < 0) {
> +        c->write_prft = c->ldash;

nit: !!, in case ldash becomes something else than a bool in the future

> +        if (c->ldash)
> +            av_log(s, AV_LOG_INFO, "Enabling Producer Reference Time element for Low Latency mode\n");

I'd say this should be VERBOSE, since a normal run with no unexpected
events should produce no log output.

Otherwise LGTM.
James Almer Feb. 26, 2020, 12:28 a.m. UTC | #2
On 2/24/2020 6:54 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-02-20 17:26:00)
>> Signed-off-by: James Almer <jamrial@gmail.com>
> 
> Commit message is now misleading since it will only enable prft if it's
> not disabled.

Sorry, i pushed this during the weekend. And, true. It's still
attempting but technically not always...

Which makes me realize i should mention this undocumented behavior in
the doxy.

>> ---
>> Now it can be overriden if you explicitly set write_prft to 0.
>>
>>  libavformat/dashenc.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>> index a52cbc9113..7032adc84d 100644
>> --- a/libavformat/dashenc.c
>> +++ b/libavformat/dashenc.c
>> @@ -1394,6 +1394,12 @@ static int dash_init(AVFormatContext *s)
>>          c->frag_type = FRAG_TYPE_EVERY_FRAME;
>>      }
>>  
>> +    if (c->write_prft < 0) {
>> +        c->write_prft = c->ldash;
> 
> nit: !!, in case ldash becomes something else than a bool in the future

The chances for that are pretty slim, since turning a bool into an int
would be an API break (true/false would stop working from the command
line, afaik). But i can change it anyway.

> 
>> +        if (c->ldash)
>> +            av_log(s, AV_LOG_INFO, "Enabling Producer Reference Time element for Low Latency mode\n");
> 
> I'd say this should be VERBOSE, since a normal run with no unexpected
> events should produce no log output.

Sure, will change in a new commit.

> 
> Otherwise LGTM.
> 

Thanks, and apologies for not waiting a bit more.
Anton Khirnov Feb. 27, 2020, 11:48 a.m. UTC | #3
Quoting James Almer (2020-02-26 01:28:48)
> On 2/24/2020 6:54 AM, Anton Khirnov wrote:
> > Quoting James Almer (2020-02-20 17:26:00)
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> > 
> > Commit message is now misleading since it will only enable prft if it's
> > not disabled.
> 
> Sorry, i pushed this during the weekend. And, true. It's still
> attempting but technically not always...
> 
> Which makes me realize i should mention this undocumented behavior in
> the doxy.
> 
> >> ---
> >> Now it can be overriden if you explicitly set write_prft to 0.
> >>
> >>  libavformat/dashenc.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> >> index a52cbc9113..7032adc84d 100644
> >> --- a/libavformat/dashenc.c
> >> +++ b/libavformat/dashenc.c
> >> @@ -1394,6 +1394,12 @@ static int dash_init(AVFormatContext *s)
> >>          c->frag_type = FRAG_TYPE_EVERY_FRAME;
> >>      }
> >>  
> >> +    if (c->write_prft < 0) {
> >> +        c->write_prft = c->ldash;
> > 
> > nit: !!, in case ldash becomes something else than a bool in the future
> 
> The chances for that are pretty slim, since turning a bool into an int
> would be an API break (true/false would stop working from the command
> line, afaik). But i can change it anyway.

I mean someone could do exactly the thing you are doing here - use -1
for default/unset.

> > 
> > Otherwise LGTM.
> > 
> 
> Thanks, and apologies for not waiting a bit more.

No problem, I should have looked more closely before replying. They were
minor comments anyway.
diff mbox series

Patch

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index a52cbc9113..7032adc84d 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -1394,6 +1394,12 @@  static int dash_init(AVFormatContext *s)
         c->frag_type = FRAG_TYPE_EVERY_FRAME;
     }
 
+    if (c->write_prft < 0) {
+        c->write_prft = c->ldash;
+        if (c->ldash)
+            av_log(s, AV_LOG_INFO, "Enabling Producer Reference Time element for Low Latency mode\n");
+    }
+
     if (c->write_prft && !c->utc_timing_url) {
         av_log(s, AV_LOG_WARNING, "Producer Reference Time element option will be ignored as utc_timing_url is not set\n");
         c->write_prft = 0;
@@ -2352,7 +2358,7 @@  static const AVOption options[] = {
     { "lhls", "Enable Low-latency HLS(Experimental). Adds #EXT-X-PREFETCH tag with current segment's URI", OFFSET(lhls), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, E },
     { "ldash", "Enable Low-latency dash. Constrains the value of a few elements", OFFSET(ldash), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, E },
     { "master_m3u8_publish_rate", "Publish master playlist every after this many segment intervals", OFFSET(master_publish_rate), AV_OPT_TYPE_INT, {.i64 = 0}, 0, UINT_MAX, E},
-    { "write_prft", "Write producer reference time element", OFFSET(write_prft), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, E},
+    { "write_prft", "Write producer reference time element", OFFSET(write_prft), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, E},
     { "mpd_profile", "Set profiles. Elements and values used in the manifest may be constrained by them", OFFSET(profile), AV_OPT_TYPE_FLAGS, {.i64 = MPD_PROFILE_DASH }, 0, UINT_MAX, E, "mpd_profile"},
     { "dash", "MPEG-DASH ISO Base media file format live profile", 0, AV_OPT_TYPE_CONST, {.i64 = MPD_PROFILE_DASH }, 0, UINT_MAX, E, "mpd_profile"},
     { "dvb_dash", "DVB-DASH profile", 0, AV_OPT_TYPE_CONST, {.i64 = MPD_PROFILE_DVB }, 0, UINT_MAX, E, "mpd_profile"},