google / ExoPlayer

This project is deprecated and stale. The latest ExoPlayer code is available in https://github.com/androidx/media
https://developer.android.com/media/media3/exoplayer
Apache License 2.0
21.7k stars 6.02k forks source link

TTML: Support missing 'extent' tag on <tt> #7361

Open fguinet opened 4 years ago

fguinet commented 4 years ago

Issue description

TtmlDecoder.parseTtsExtent does not support extent defined in pixel such as :

        <layout>
            <region tts:extent="100% 100%" tts:origin="0px 0px" xml:id="region1"/>
        </layout>

The region in that case is not considered as : Log.w(TAG, "Ignoring non-pixel tts extent: " + ttsExtent); (line 244) which leads to : Log.w(TAG, "Ignoring region with missing tts:extent: " + regionOrigin); (line 336)

It looks from the specification tts:extent that "100% 100%" could be a valid answer.

Version of ExoPlayer being used

I have looked at both 2.11.3 and 2.11.4 version

icbaker commented 4 years ago

I think there is something not quite right here, but it's what you've quite described.

Confusingly TtmlDecoder#parseTtsExtent is only used to parse the extent of tt nodes, which must be in px (or auto or contains):

If a tts:extent attribute is specified on the tt element, then the specified value is restricted to one of the following: (1) the auto keyword, (2) the contain keyword, or (3) two specifications, where these specifications are expressed as non-percentage, definite lengths using pixel units. https://www.w3.org/TR/2018/REC-ttml2-20181108/#style-attribute-extent

When parsing a tts:extent on region we do allow percentages: https://github.com/google/ExoPlayer/blob/7d3f54a375fac04a746ca76a8f2e1ad32c8b45b2/library/core/src/main/java/com/google/android/exoplayer2/text/ttml/TtmlDecoder.java#L367

What I think is going wrong is when we don't find tts:extent on the tt node, we then ignore all extent and origin properties on regions. The spec says what to do if extent is missing on tt:

If no tts:extent attribute is specified, then the spatial extent of the root container region is determined by rules specified in H Root Container Region Semantics.

H.2 then says:

If the value of the tts:extent attribute is specified on the tt element and consists of two pixel-valued expressions, then these two expressions denote the width and height of the root container region, and, consequently, determine its collective resolution; otherwise, the collective resolution of the root container region is determined (arbitrarily) by the document processing context in a manner that respects the resolved values of SAR as determined by H.1 Aspect Ratios above.

My reading of this is that in these cases we should basically assume the tt is "full screen".

fguinet commented 4 years ago

Please note that even if the region defined the extent like this : <region tts:extent="800px 600px" tts:origin="0px 0px" xml:id="region1"/> The extent is not condidered. I would suggest to patch the code as followed : TtmlDecoder > parseRegionAttributes Replace : Log.w(TAG, "Ignoring region with missing tts:extent: " + regionOrigin); with

ttsExtent = parseTtsExtent(xmlParser);
if (ttsExtent == null) {
          Log.w(TAG, "Ignoring region with missing tts:extent: " + regionOrigin);
}
icbaker commented 4 years ago

Does your <tt> tag have tts:extent defined on it (in px)? If not, that's why the tts:extent on your <region> is currently ignored. That's what this issue is tracking - to allow TTML files that don't define tts:extent on the <tt> tag.

If your <tt> tag does have tts:extent defined then I'd expect a <region tts:extent=...> to be correctly detected (either % or px). If that's not the case please can you provide the TTML file you're using and I'll take a look (either uploaded here or sent to exoplayer.dev@gmail.com with subject "Issue #7361").

Your proposed fix is incorrect - parseTtsExtent() is only intended to be used to parse tts:extent from the <tt> tag, not <region> or others. It's arguably a somewhat poorly named method :)

fguinet commented 4 years ago

No my <tt> tag does not have tts:extent but your right my <region> tag has one :

 <layout>
    <region tts:extent="800px 600px" tts:origin="0px 0px" xml:id="region1"/>
  </layout>

The proposed fix is working fine in my case:

if (ttsExtent == null) {
         ttsExtent = parseTtsExtent(xmlParser);
         if (ttsExtent == null) {
                  Log.w(TAG, "Ignoring region with missing tts:extent: " + regionOrigin);
         }
}

Explanation : The ttsExtent is firstly null because nothing is declared in tt so I get the extent from the region

Here is the ttml file for your analysis : dvb_sub.ttml.zip