Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update LegacyConversions.java #1967

Open
wants to merge 1 commit into
base: release
Choose a base branch
from

Conversation

AndresBC-123
Copy link

I have fixed a possible bug when mixing displayTitle with title which causes displayTitle to appear when displayTitle should appear in legacy MediaControllers.

@marcbaechinger
Copy link
Contributor

marcbaechinger commented Dec 11, 2024

Thanks for your pull request.

Can you elaborate a bit on what the bug is about, what the symptoms of the bug is and how this change fixes it?

I'm asking because this is implemented in a specific way right now that takes into account expected behaviour for instance of the AVRPC also.

After your change I think displayTitle never makes it into the legacy metadata object. So if this is correct, then we should understand this and know for which use cases it works better that before didn't work. I think never using displayTitle doesn't feel right to me specifically when looking at L857 and the logic around PREFERRED_DESCRIPTION_ORDER.

@marcbaechinger marcbaechinger self-assigned this Dec 11, 2024
@marcbaechinger marcbaechinger self-requested a review December 11, 2024 10:20
@AndresBC-123
Copy link
Author

Hi, thanks for your prompt reply.
It may not be a bug but expected behavior, but I think it's inconsistent.
I don't think it's a good idea to assign the displayTitle to the title since they are fields that, although they may be similar or even the same in most cases, do not have to be.
In legacy MediaControllers, for example in Android Auto, the title appears on the main playback screen, however the user will see the displayTitle, which, in my opinion, is not correct.
Testing, I saw this behavior also occurs in the media session notification.
The goal is that the title assignment does not change throughout the code and is assigned correctly in L878.

@marcbaechinger
Copy link
Contributor

Hmm, I actually have changed that on request of Auto and AAOS people and its reviewed by them. I can make them look at your request. For our information:

In legacy MediaControllers, for example in Android Auto, the title appears on
the main playback screen, however the user will see the displayTitle, which, in my opinion, is not correct.

I don't understand this. How does the user see 'displayTitle' when the 'title' is displayed?

Can you let me know what the intention of a developer is who sets title and displayTitle differently in a Media3 MediaMetadata instance?

With the current implementation before your change, setting the displayTitle would use that. If a developer doesn't want this, they can not set the displayTitle because then the title is used. This can be an issue if the developer wants to set the displayTitle but it should not be used when converting to the legacy MediaMetadataCompat. Which use case would suffer when not using displayTitle in such a case?

I will check with the Auto team also to discuss this.

@AndresBC-123
Copy link
Author

I don't understand this. How does the user see 'displayTitle' when the 'title' is displayed?

The title is displayed, but since it has been assigned displayTitle, the contents of displayTitle will be seen in Android Auto.
NOTE: This also occurs in the MediaStyled media session notification. Please, check it.

Can you let me know what the intention of a developer is who sets title and displayTitle differently in a Media3 MediaMetadata instance?

Here I am not referring to the developer but to the content/metadata of an audio or video file.

With the current implementation before your change, setting the displayTitle would use that. If a developer doesn't want this, they can not set the displayTitle because then the title is used. This can be an issue if the developer wants to set the displayTitle but it should not be used when converting to the legacy MediaMetadataCompat. Which use case would suffer when not using displayTitle in such a case?

I don't think there is any use case that would suffer any problems in such a case.

@marcbaechinger
Copy link
Contributor

Here I am not referring to the developer but to the content/metadata of an audio or video file.

Now, I'm confused.

You send a pull for MediaDescriptionCompat convertToMediaDescriptionCompat(MediaItem item, @Nullable Bitmap artworkBitmap). That method gets passed in a MediaItem and produces a MediaDescriptionCompat.

This is called in three places. The first two:

  • From MediaControllerImplLegacy to create queue items passed into controllerCompat.addQueueItem(MediaDescriptionCompat) when accessing a legacy session.
  • From MediaSessionLegacyStub to create queue items to be served to legacy controllers accessing a Media3 session.

these two are not used by Android Auto/AAOS I think.

The remaining callsites are calling convertToMediaDescriptionCompat indirectly through convertToBrowserItem from MediaLibraryServiceLegacyStub. That's what is involved in Auto regarding your change I think. When Auto/AAOS accesses a MediaLibraryService as a mediaBrowserCompat and for instance gets children that are MediaBrowserCompat.MediaItem.

These come from what an app returns from for instance MediaLibrarySession.Callback.onGetChildren():LibraryResult<List<MediaItem>>.

Here I am not referring to the developer but to the content/metadata of an audio or video file.

How do you think is the metadata from the audio or video file leaking into these media items that are returned from the callback methods?

NOTE: This also occurs in the MediaStyled media session notification.

That's another case. If I'm not mistaken, here the media notification indeed may receive metadata from onMediaMetadatdaChanged() that is converted to the legacy metadata and then System UI uses this.

So if we can find evidence that some metadata from either in-band metdata (vid/audio files), ICY headers or other sources produces a displayTitle that we merge into the metadata emitted by Player.Listener.onMediaMetadatdaChanged(), then that could happen.

But we wouldn't need to change LegacyConversions.convertToMediaDescriptionCompat but rather look into LegacyConversions.convertToMediaMetadataCompat() I think which is implemented differently.

Sorry, for asking this again. Can you elaborate a bit on what the bug is about, what the symptoms of the bug is and how this change fixes it? I'm afraid I insist, so please accept my apologies. I want to understand the change first, before starting to review.

@AndresBC-123
Copy link
Author

Let's consider this scenario:
I have an mp3 audio file stored on an Android phone.
My app accesses its metadata using the MediaStore database and creates a MediaItem.
During this process, my app sets the song title field in the metadata to title (e.g., "Viva la vida") and sets the file name to displayTitle (e.g., "01 Viva la vida.mp3"). I think this way of assignment is the most logical.
When I play this MediaItem using ExoPlayer and connect the phone to Android Auto, the Android Auto playback screen shows the file name instead of the song title.
I think it makes more sense to show the title of the song.

If displayTitle!=null, why does the MediaDescriptionCompat returned by convertToMediaDescriptionCompat() contain displayTitle instead of title? It is assigned for no reason on line 850, since there is no nullity check for title.

As far as I can see, line 850 of LegacyConversions is the only place where diplayTitle is assigned to title, where the variables are swapped for no reason.

These come from what an app returns from for instance MediaLibrarySession.Callback.onGetChildren():LibraryResult<List>.

From what I've been able to verify, Android Auto seems to do what you're saying.

That's another case. If I'm not mistaken, here the media notification indeed may receive metadata from onMediaMetadatdaChanged() that is converted to the legacy metadata and then System UI uses this.

I have also seen this effect in the notification so it somehow reads information from the same place.

Regarding convertToMediaMetadataCompat(), it is a very simple method, just variable assignments, I don't think it needs to be modified at all.

@marcbaechinger
Copy link
Contributor

my app sets the song title field in the metadata to title (e.g., "Viva la vida") and
sets the file name to displayTitle (e.g., "01 Viva la vida.mp3").

In this case, I think your app should not do that.

Many thanks for your thoughts. My offers stands under the conditions that I'm ok to repeat: Can you elaborate a bit on what the bug is about, what the symptoms of the bug is and how this change fixes it? Random unreasonable app behavior is not a bug to add this as a side note.

It's fine with me if you respond again, but without you bringing up something substantial, I won't respond any further I'm afraid. I suggest to save your time in that case and for instance do a walk or something. I do snowboarding and I look forward to go to the mountains the first time this year. Good for stretching my mind every now and then.

@tonihei
Copy link
Collaborator

tonihei commented Dec 13, 2024

my app sets the song title field in the metadata to title (e.g., "Viva la vida") and
sets the file name to displayTitle (e.g., "01 Viva la vida.mp3").
In this case, I think your app should not do that.

Adding to that, and maybe explaining the confusion: The display title is meant to be the preferred string that is displayed to the user instead of the regular title. This is pretty much the reason we prefer the displayTitle field over title if both are set. And I guess from your example you may want to set the two fields the other way round?

@AndresBC-123
Copy link
Author

As previously stated, the core issue is:
When displayTitle is not null, why does convertToMediaDescriptionCompat() return a MediaDescriptionCompat containing displayTitle instead of title?
Line 850 of LegacyConversions prioritizes displayTitle by assigning it to title, even if metadata.title is present and contains a value. This effectively ignores the value of metadata.title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants