Re: [FFmpeg-devel] [PATCH v3 1/2] fftools/resources: clean up and improve resource manager build system

["softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Ramiro Polla
> Sent: Freitag, 30. Mai 2025 12:52
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v3 1/2] fftools/resources: clean up and improve
> resource manager build system
> 
> - move .gitignore entries to main .gitignore;
> - move vpath directives to main Makefile;
> - remove superfluous comments;
> - turn css minification sed command into a one-liner;
> - deduplicate targets depending on CONFIG_RESOURCE_COMPRESSION;
> - introduce common .res pattern for resource files;
> - remove RESOURCEOBJS noop from common.mak (it was never populated);
> - add fftools/graph/Makefile;
> - rename OBJS-resman to RESMAN-OBJS for consistency;
> - move graph.{css,html} to fftools/graph/graphprint.{css,html};
> - disable dependency checking for resource files, to prevent spurious
>   rebuilds;
> - generate resources list at build-time based on all resource files;
> - the resource manager now uses the resource filename instead of an ID.
> 
> Adding resource files now works from any subdir. Suppose you want to
> add a resource file named "foo.html", then all you have to do is:
> 
> OBJS-$(CONDITION) += foo.html.res.o
> 
> To access the resource, you retrieve it by its name:
> 
> data = ff_resman_get_string("foo.html");
> ---

Hi Ramiro,

here's my review:

1. General

First of all, I think there are a bit too many different changes at once
in this patch. It would be better to have each kind of change 
separate and then apply it uniformly to both, .ptx and .res compression
(where applicable). Having different logic for both seems pretty odd, 
that's why I had talked to Timo about his opinion and we had agreed to
still keep the ptx files, but treat the others as intermediate.
That applied to the .SECONDARY and CCDEPS setting, and in your patch
it would also apply to the replacement of the ifdef blocks with the 
inline condition. I'm not against this change, even though will make
it quite hard for others to understand, when dealing with it in 
the future.


2. Moving vpath to the top level Makefile

The reasons why I didn't add them in the top Makefile is that there are 
other .html and .css files in subfolders to which the vpath directive
would apply when added to the top Makefile.
I don't know whether this could cause any regressions, to the change might
or might not be okay.


3. Downlevel Makefile dependency

Your changes introduce a (logical) dependency in a level-3 Makefile on 
another level-3 Makefile, i.e. fftools/resources/Makefile controls 
what's happening to files in fftools/graph/Makefile.


4. fftools/resources/Makefile: #.SECONDARY line

It does not retain min file when uncommented and resource compression is
enabled.


5. Is this a suitable pattern for a generalized resource handling?

You recently asked me about a plan for this, and I said I don't have
one yet and that's why I wanted to keep things flexible for now.
It's not that I haven't thought about it - it's not as easy as one
might think.
The solution you are proposing doesn't seem suitable to fulfill these
requirements - for the following reasons:

- It doesn't allow to include different resources for ffprobe and
  ffmpeg - it would include all resourced in both.

- Putting resource files into the individual folders of the code that
  is using them is not a good idea, because it doesn't allow them to 
  be re-used by other features. E.g.: a feature in ffprobe might need
  the same html as graphprint but a different css.
  That's why I have put them all in the same directly, which is also
  better for clarity.

- You are removing the resource ID definitions and are relying on 
  string matching only. Of course, this simplifies the code, but I 
  had decided against doing so, in order to have a compile-time
  mechanism that guarantees that the build includes all the resources
  that are needed by the included features.
  Otherwise, it might too easily happen that builds are omitting
  resources and it would be discovered much later only by a runtime
  error

6. common.mak Cleanup

Please also see the second path in my update patchset for futher
way to consolidate and simplify common.mak rules.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14627


Thanks,
sw

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".